From 75facb4d67d026d312a99252abc7a6c420b864fb Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Wed, 26 Aug 2026 09:08:50 +0200 Subject: [PATCH] fix(gallery): stop the lightbox loading originals to display a photo (#1166) (#1175) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(gallery): stop the lightbox loading originals to display a photo (#1166) Stable twin of #1169. The lightbox read preview_url, which the server only emits once an admin has flipped lightbox_preview_enabled — off by default. So a stock install fell straight through to url, the untouched original: a reporter measured 16.5 MB for a photo whose preview is 345 KB. The lightbox renders its neighbours too, so opening one photo pulled three originals. slideshow_url is the same /preview/:id URL, watermark query included, and has been emitted unconditionally for images since #1015. Preferring it fixes every existing install with no migration and no admin action. Two other surfaces bypass PhotoLightbox entirely and had the same bug: - premium galleries build their own slides with `src: photo.url`. Fixing that also required carrying the photo id on the slide, because the download handler recovered the photo by matching slide.src against photo.url — a derivative src would have made Download a silent no-op. - the Story layout rendered the full original as its GRID TILE, at object-cover in a small card, and its hero rendered one as a full-bleed background when hero_url exists for exactly that. Cards now use the preview tier (not the thumbnail: thumbnail_fit is seeded to 'cover', so a thumbnail would be cropped a second time and reframe every photo) and only load once within 200px of the viewport, since every card mounts at page load. GIF, APNG and PNG keep the original: generatePreviewImage encodes JPEG, which has neither a second frame nor an alpha channel. The backend fix that removes this list is the next commit in this stack. Divergence from the main twin: no responsive `?w=` tiers. #1095 is main-only, so `lightboxImageUrl` here selects a URL and nothing more. It lives in `imageTiers.ts` under the same path main uses, so that backporting #1095 later merges into this file rather than landing beside it. Verified on this branch: 8 new tests; frontend suite 21 files / 113 tests, tsc clean. * fix(gallery): make the Story hero fix actually work on external galleries (#1166) External review. Same two fixes as the main twin. hero_url was inert for external media. ensureHeroImage only ever called resolvePhotoStorageKey, which returns null for external/reference photos by design — and that null was 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 stayed invisible until this PR pointed the Story hero at hero_url: on a managed gallery that is a real saving, on a reference-mode gallery it quietly changed nothing. Needed one extra piece here that main already had: generateHeroImage on this branch ignores outputBasename and always derives the key from the source basename, so two events referencing the same NAS filename would clobber each other's hero. It now honours the option, matching generateThumbnail and generatePreviewImage. The format bypass trusted mime_type, which is not trustworthy: migration 039 backfilled every pre-existing photo to image/jpeg regardless of what it was, and adminExternalMedia inserts rows with no mime_type at all — so a mislabelled PNG sailed past the guard and came back flattened. It now checks the filename extension as well. * test(gallery): the hero fixture follows the root-relative relpath contract (#1166) Same fix as the main twin: external_relpath has been resolved from EXTERNAL_MEDIA_ROOT rather than from event.external_path since #1163 landed, and this fixture still carried the base-relative form, so the two tests stopped resolving the moment that stack merged. Production was never affected. --------- Co-authored-by: Paul Nothaft --- .../services/ensureHeroImage.external.test.js | 116 ++++++++++++++++++ backend/src/services/imageProcessor.js | 67 ++++++++-- .../src/components/gallery/PhotoLightbox.tsx | 16 ++- .../__tests__/lightboxImageUrl.test.ts | 97 +++++++++++++++ frontend/src/components/gallery/imageTiers.ts | 64 ++++++++++ .../gallery/layouts/GalleryPremiumLayout.tsx | 22 +++- .../gallery/layouts/story/StoryHero.tsx | 9 +- .../gallery/layouts/story/StoryPhotoCard.tsx | 41 ++++++- .../features/settings/tabs/ThumbnailsTab.tsx | 6 +- frontend/src/i18n/locales/de.json | 6 +- frontend/src/i18n/locales/en.json | 6 +- frontend/src/i18n/locales/fr.json | 6 +- frontend/src/i18n/locales/sl.json | 6 +- 13 files changed, 420 insertions(+), 42 deletions(-) create mode 100644 backend/__tests__/services/ensureHeroImage.external.test.js create mode 100644 frontend/src/components/gallery/__tests__/lightboxImageUrl.test.ts create mode 100644 frontend/src/components/gallery/imageTiers.ts 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 ? ( +