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 618718e5..5539a1f3 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -377,7 +377,10 @@ async function generateVideoPlaceholder(originalFilename, options = {}) { * Outputs a 1920x1080 image suitable for full-width hero sections */ async function generateHeroImage(imagePath, options = {}) { - const filename = path.basename(imagePath); + // outputBasename lets callers disambiguate sources that share a basename — + // two events referencing the same NAS filename would otherwise clobber each + // other's hero. Same contract as generateThumbnail and generatePreviewImage. + const filename = options.outputBasename || path.basename(imagePath); const heroFilename = `hero_${filename}`; const heroRelKey = path.posix.join('heroes', heroFilename); const storage = getStorage(); @@ -459,16 +462,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; } @@ -480,7 +480,56 @@ async function ensureHeroImage(photo) { logger.warn(`Invalid hero image detected for photo ${photo.id}, regenerating...`); } - const newHeroPath = await withLocalCopy(sourceKey, (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 heroIsExternal = photo.source_origin === 'external' || photo.source_origin === 'reference'; + + let newHeroPath; + if (heroIsExternal) { + // 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, (localPath) => generateHeroImage(localPath, { regenerate: true }) ); diff --git a/frontend/src/components/gallery/PhotoLightbox.tsx b/frontend/src/components/gallery/PhotoLightbox.tsx index 8968a16c..102c1c19 100644 --- a/frontend/src/components/gallery/PhotoLightbox.tsx +++ b/frontend/src/components/gallery/PhotoLightbox.tsx @@ -11,6 +11,7 @@ import { FeedbackIdentityModal } from './FeedbackIdentityModal'; import { VideoPlayer } from './VideoPlayer'; import { useGuestIdentityOptional } from '../../contexts/GuestIdentityContext'; import { useFeedbackLimitModal } from '../../hooks/useFeedbackLimitModal'; +import { lightboxImageUrl } from './imageTiers'; interface PhotoLightboxProps { photos: Photo[]; @@ -838,12 +839,10 @@ export const PhotoLightbox: React.FC = ({ /> ) : ( = ({ onClick={handleImageClick} > { + it('uses the preview tier when the admin opted in', () => { + expect(lightboxImageUrl({ ...PHOTO, preview_url: '/api/gallery/g/preview/47' })) + .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. + expect(lightboxImageUrl(PHOTO)).toBe('/api/gallery/g/preview/47'); + }); + + it('never serves the original while a derivative exists', () => { + for (const preview_url of [null, undefined, '', '/api/gallery/g/preview/47']) { + expect(lightboxImageUrl({ ...PHOTO, preview_url })).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({ ...PHOTO, 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, + // so the wm parameter is on whichever one is used — losing it would serve + // an unwatermarked frame to a gallery that asked for one. + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47?wm=3', + preview_url: null, + slideshow_url: '/api/gallery/g/preview/47?wm=3', + })).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_type) => { + // generatePreviewImage encodes JPEG: no second frame, no alpha channel. + expect(lightboxImageUrl({ ...PHOTO, mime_type })).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. + 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', + })).toBe('/api/gallery/g/photo/47'); + }); + + it('catches one with no mime_type at all, as external imports write them', () => { + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url: null, + slideshow_url: '/api/gallery/g/preview/47', + filename: 'animation.gif', + })).toBe('/api/gallery/g/photo/47'); + }); + + it('still uses the preview tier for ordinary still formats', () => { + for (const mime_type of ['image/jpeg', 'image/webp', undefined]) { + expect(lightboxImageUrl({ ...PHOTO, mime_type })).toBe('/api/gallery/g/preview/47'); + } + }); +}); diff --git a/frontend/src/components/gallery/imageTiers.ts b/frontend/src/components/gallery/imageTiers.ts new file mode 100644 index 00000000..6bcddc57 --- /dev/null +++ b/frontend/src/components/gallery/imageTiers.ts @@ -0,0 +1,64 @@ +/** + * Which rendition a surface should display. + * + * On `main` this file also carries the responsive tier machinery (#1095) — + * `previewUrlForViewport`, `thumbnailUrlForTile`, the width tables. None of + * that is on this branch, so URLs here are used as the server emits them. The + * filename matches main deliberately, so that when #1095 is backported it + * merges into this file rather than landing beside it. + */ + +/** + * 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 (#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; +}): string { + // Animated and transparent formats keep the original. generatePreviewImage + // encodes JPEG, which has neither a second frame nor an alpha channel, so + // routing these through the preview tier would replace an animation with its + // first frame and flatten transparency onto a solid background — a + // regression the toggle-off default never had. + // + // PNG is in the list because that is where transparency is the norm, and + // because an APNG is normally reported as image/png rather than image/apng. + // Animated or alpha WebP declares image/webp exactly like an ordinary still + // and cannot be told apart from MIME. + // + // The proper fix is backend-side, encoding WebP for alpha or multi-page + // sources; when that lands this list goes away entirely. + // 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 photo.preview_url || photo.slideshow_url || photo.url; +} diff --git a/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx b/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx index 9be0335f..5d1ff0dd 100644 --- a/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx +++ b/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx @@ -27,6 +27,7 @@ import { useDownloadPhoto } from '../../../hooks/useGallery'; import { toast } from 'react-toastify'; import './GalleryPremiumLayout.css'; +import { lightboxImageUrl } from '../imageTiers'; interface PhotoCardProps { photo: Photo; @@ -256,7 +257,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 recovers the photo by id, because matching + // slide.src against photo.url stops working the moment src is a + // derivative — Download would silently do nothing. + photoId: photo.id, alt: photo.filename, width: photo.width || 1200, height: photo.height || 800, @@ -371,10 +382,15 @@ export const GalleryPremiumLayout: React.FC = ({ } }, [selectedPhotos, slug, t]); - 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..5aa61681 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. + 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 ? ( +