* feat(gallery): responsive grid thumbnails (#1095) The half of #1095 that #1099 deliberately left out. Grid tiles are ~175 CSS px at the mobile 2-column default — about 530 device px on a DPR-3 phone — so the 300px thumbnail is upscaled ~1.8x and faces visibly mush. Backend mirrors the preview tiers exactly: ?w= on the gallery thumbnail route, whitelisted to 300/600/900, cached by width in storage, never written to photos.thumbnail_path, and keyed by photo id for every source type — basenames are not unique across events and a tier is served from a cache hit without re-reading the source, which is how the preview tiers nearly leaked one gallery's photo into another. The tier is in the ETag, or a client holding the 300px file gets a 304 for its 600px request. Cleanup and regenerate invalidation are wired the same way. generateThumbnail now takes width/height overrides; it keeps the configured `fit`, because the grid renders with object-cover and tiers that were framed differently would visibly jump as the viewport changes. The srcset only advertises tiers the SOURCE can fill. Thumbnails are generated withoutEnlargement, so a 400px original asked for 900 comes back at 400 — advertising "900w" would have the browser pick that candidate and upscale it, which is the reported softness made worse. That exact trap is why this was held back from #1099; the photo's own dimensions are now the guard, measured on the SHORT edge because thumbnails are square and a 4000x600 panorama can still only fill a 600 tile. A source that clears only one tier gets no srcset at all rather than a single pointless candidate. Two things this surfaced, both worth knowing separately: `npx tsc --noEmit` type-checks NOTHING in this project — the root tsconfig is `files: []` with project references, so the real command is `tsc -b`, which is what build:check runs. Under tsc -b the repo has 43 files with pre-existing type errors; this branch adds none, and the one error in a file I touched (PeopleManagerModal:91) is on main already and unrelated to the line I changed. * fix(gallery): wire grid tiers into the component that actually renders The srcSet landed in PhotoGrid.tsx, which nothing imports — GalleryView renders PhotoGridWithLayouts, and every grid layout funnels its tile through the shared PhotoCard. The frontend half of #1095 shipped nothing. Moved to PhotoCard, and switched from srcSet to a single sized URL, the same shape PhotoLightbox already uses for preview tiers. AuthenticatedImage fetches its src with the gallery bearer token and renders the blob; an <img> carrying a w-descriptor srcSet ignores src entirely, so that fetch would have been discarded and the browser would have issued its own — unauthenticated, and resolved against the page origin rather than the configured API host. One URL keeps the auth path and halves the requests. The tier comes from the tile's measured width via the IntersectionObserver entry, read on the same render that reveals the image so nothing is fetched twice. Column counts differ per layout and shift again with thumbnailScale, so the breakpoint table is only a fallback. Also closes what the tier cache leaked or served stale: - ensureThumbnailAtWidth short-circuits videos. Their thumbnail is a poster frame, so the tier path handed the video file to Sharp — after downloading it in full on S3, uncached, once per request. - The ETag names the tier actually served, not the one requested. A fallback to the canonical thumbnail was caching a 300px image under a 900px key. - Tier height scales from the configured aspect ratio instead of forcing a square; with fit:'cover' a 300x200 canonical and a 600x600 tier are two different crops and the photo reframed between tiers. - The canonical short-circuit compares against the configured thumbnail_width, not the 300 default, so a 600px install stops generating duplicate tiers. - Tier invalidation on /admin/thumbnails/regenerate, above the local-file check that skips S3 and external rows. - Tier cleanup in replacePhoto and deleteEventCascade. Both derive keys from the photo row, so the rows have to be read before they change or vanish. Preview tiers had the same two holes and are swept alongside. The clamp no longer drops a tier when the source falls between them: a 400px short edge asked for 600 returns all 400 pixels, where clamping to 300 threw 100 of them away. Backend 18 tier tests, frontend 22. Full suites green: 293 backend across the touched areas, 185 frontend, build clean, no new type errors. * fix(gallery): measure the tile, and stop regenerating the w300 tier Follow-up to the review of #1095. Closes the three items left open there, plus a defect the previous commit introduced. **The w300 tier regenerated on every request.** Decoupling the canonical short-circuit from the hardcoded 300 left generateThumbnail still tagging against DEFAULT_THUMBNAIL_WIDTH. On an install with thumbnail_width=600 a w=300 request wrote `thumb_<name>` while the caller probed for `thumb_w300_<name>`: the cache never hit, so every request re-downloaded the original and ran Sharp, and the file it left behind was in no cleanup list. The tag now follows the configured width, and thumbnailTierKeys lists all three widths — which one is canonical is a setting, so excluding 300 stranded exactly the file a 600-configured install generates. **The tier is chosen from the tile's measured width.** The observer entry only exists for `lazy` cards, and Mosaic, Masonry and Timeline don't pass it — Mosaic is 1-up on mobile where Grid is 2-up, so they are the layouts a breakpoint guess gets most wrong. Measured in a layout effect and gated: the image is not rendered until the width is known, so AuthenticatedImage never mounts with a src it has to replace. Attaching the observer ref unconditionally instead refetches every tile, since React flushes passive effects before the sync re-render a layout effect triggers — removing the gate makes the new single-request test fail, which is how that was confirmed rather than assumed. **Gallery Premium has its own card** and never reached the shared one, so its tiles kept pulling the canonical thumbnail. MasonryPhotoAlbum already hands the laid-out width to the render prop, so it needed no measurement. **Event rename orphaned tiers.** The key embeds the basename, so the DB update is the point past which the old keys cannot be derived. Dropped inside the filename-changed branch, not the loop body: unconditional would fire four storage deletes per photo on every rename, 20k calls against S3 for a 5,000-photo event that merely had its slug adjusted. Preview tiers had the same hole and are swept alongside. Carousel is the seventh layout and deliberately gets no tiering: its filmstrip thumbs are 80 CSS px, under the canonical 300 even at DPR 3. Tests: first PhotoCard suite (6), backend tier suite 21. Both new behaviours mutation-checked — reverting the width tag, the render gate, the measurement, or the rename sweep each fails a test. Full suites green: 298 backend across the touched areas, 191 frontend, build clean, no new type or lint findings. * fix(gallery): mount masonry cards once, into a measured layout Found while capturing screenshots for this PR, by attributing every thumbnail request to a photo id rather than eyeballing the grid. Masonry columns mode starts at 3 columns and runs its greedy distribution off a hardcoded 300px estimate until the container has been measured. Cards mounted into that guess are torn down when it settles — photos move to a different parent column, so React unmounts them — and since #1095 each mount picks its tier from its own width, the two mounts request two DIFFERENT urls. Measured on a 1440px desktop, production build, 62 photos: before 45 photos fetched at canonical AND w600, 17 stuck on w600 107 requests after 62 photos, canonical only, 62 requests Mobile was already landing on one tier either way, so both mounts produced the same url and the second was a cache hit — which is why it looked clean and the desktop case did not. The fix is the gate the rows/justified mode in this same file already applies for the same reason (line 346): hold the cards back until containerWidth is known. Only columns mode was missing it. Grid and Justified take their column counts from CSS breakpoints, so they have no transient measured value to discard and are unaffected. Worth noting this was NOT visible on main: without tiering both mounts request the same url, so the browser cache absorbs the duplicate. Tiering is what turns a harmless remount into a second download — the regression is this PR's, which is why it is fixed here rather than deferred. Frontend suite 194 passed (3 new). Mutation-checked: removing the gate fails the mount-once and placeholder tests. --------- Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
00776234fd
commit
887bdbe6e5
@@ -1,7 +1,8 @@
|
||||
import React, { useCallback, useEffect, useRef, useState } from 'react';
|
||||
import React, { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react';
|
||||
import { Download, Maximize2, Check, MessageSquare, Heart } from 'lucide-react';
|
||||
import { useInView } from 'react-intersection-observer';
|
||||
import { AuthenticatedImage } from '../common';
|
||||
import { thumbnailUrlForTile } from './imageTiers';
|
||||
import { FeedbackIdentityModal } from './FeedbackIdentityModal';
|
||||
import { feedbackService } from '../../services/feedback.service';
|
||||
import { useGuestIdentityOptional } from '../../contexts/GuestIdentityContext';
|
||||
@@ -193,6 +194,34 @@ export const PhotoCard: React.FC<PhotoCardProps> = ({
|
||||
});
|
||||
const inView = !lazy || observedInView;
|
||||
|
||||
// Tile width for the responsive tier (#1095), measured rather than inferred.
|
||||
// The observer entry only exists for `lazy` cards, and Mosaic, Masonry and
|
||||
// Timeline do not pass it — Mosaic is 1-up on mobile where Grid is 2-up, so
|
||||
// those are exactly the layouts a breakpoint guess gets most wrong.
|
||||
//
|
||||
// Gated: the image is not rendered until this has run, so AuthenticatedImage
|
||||
// never mounts with a src it would have to replace. Attaching the observer
|
||||
// ref unconditionally instead would refetch every tile — React flushes
|
||||
// passive effects before the synchronous re-render a layout effect triggers,
|
||||
// so the fetch fires once with the fallback and again with the measurement.
|
||||
//
|
||||
// useLayoutEffect, so the extra commit lands before paint and the skeleton
|
||||
// branch below is never actually seen. One reflow per commit, not per card:
|
||||
// nothing writes to the DOM between the reads, so the browser batches them.
|
||||
const containerRef = useRef<HTMLDivElement | null>(null);
|
||||
const [tile, setTile] = useState<{ width: number | null } | null>(null);
|
||||
const setContainerRef = useCallback((node: HTMLDivElement | null) => {
|
||||
containerRef.current = node;
|
||||
if (lazy) ref(node);
|
||||
}, [ref, lazy]);
|
||||
|
||||
useLayoutEffect(() => {
|
||||
if (!inView || tile) return;
|
||||
// 0 means not laid out (a hidden tab, say), not a 0px tile — null falls
|
||||
// back to the viewport estimate rather than pinning the smallest tier.
|
||||
setTile({ width: containerRef.current?.offsetWidth || null });
|
||||
}, [inView, tile]);
|
||||
|
||||
const showFeedbackActions = feedbackEnabled && Boolean(feedbackOptions);
|
||||
|
||||
const overlayVisibilityClass = overlayVisible
|
||||
@@ -322,17 +351,32 @@ export const PhotoCard: React.FC<PhotoCardProps> = ({
|
||||
</button>
|
||||
) : null;
|
||||
|
||||
// Responsive grid tier (#1095). Applied here rather than in each layout
|
||||
// because six of the seven funnel their tile through this one image; the
|
||||
// seventh, Carousel, renders 80px filmstrip thumbs that the canonical 300
|
||||
// already covers at DPR 3.
|
||||
//
|
||||
// Only when the src IS the thumbnail route: layouts fall back to photo.url
|
||||
// when thumbnail_url is null, and ?w= on the original-photo route means
|
||||
// something else. Videos are excluded because their thumbnail is a poster
|
||||
// frame from the video pipeline — the tier route would hand the video file
|
||||
// itself to Sharp.
|
||||
const isVideo = photo.media_type === 'video' || photo.type === 'video';
|
||||
const tileSrc = !isVideo && photo.thumbnail_url && imageProps.src === photo.thumbnail_url
|
||||
? (thumbnailUrlForTile(photo.thumbnail_url, photo, tile?.width) ?? imageProps.src)
|
||||
: imageProps.src;
|
||||
|
||||
return (
|
||||
<div
|
||||
ref={lazy ? ref : undefined}
|
||||
ref={setContainerRef}
|
||||
className={className}
|
||||
style={lazy ? { ...style, opacity: !inView && fadeInWhenVisible ? 0 : 1 } : style}
|
||||
onClick={handlePhotoClick}
|
||||
{...containerProps}
|
||||
>
|
||||
{inView ? (
|
||||
{inView && tile ? (
|
||||
<>
|
||||
<AuthenticatedImage {...imageProps} />
|
||||
<AuthenticatedImage {...imageProps} src={tileSrc} />
|
||||
|
||||
{beforeOverlay}
|
||||
|
||||
|
||||
@@ -0,0 +1,139 @@
|
||||
/**
|
||||
* Grid tile sizing (#1095).
|
||||
*
|
||||
* The tier has to be decided from the tile's real width, and it has to be
|
||||
* decided BEFORE the image is requested. Both halves are easy to break without
|
||||
* anything looking wrong: the picture still renders, just at the wrong size, or
|
||||
* at the right size after fetching the wrong one first.
|
||||
*/
|
||||
import React from 'react';
|
||||
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
|
||||
import { render, screen } from '@testing-library/react';
|
||||
|
||||
import { PhotoCard } from '../PhotoCard';
|
||||
import type { Photo } from '../../../types';
|
||||
|
||||
// AuthenticatedImage really fetches; all this test cares about is the src it
|
||||
// was handed, and how many distinct ones it saw.
|
||||
const seenSrcs: string[] = [];
|
||||
vi.mock('../../common', () => ({
|
||||
AuthenticatedImage: ({ src, alt }: { src: string; alt?: string }) => {
|
||||
seenSrcs.push(src);
|
||||
return <img data-testid="tile" src={src} alt={alt} />;
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock('../../../contexts/GuestIdentityContext', () => ({
|
||||
useGuestIdentityOptional: () => null,
|
||||
}));
|
||||
|
||||
const PHOTO = {
|
||||
id: 7,
|
||||
filename: 'IMG_0001.jpg',
|
||||
url: '/api/gallery/x/photo/7',
|
||||
thumbnail_url: '/api/gallery/x/thumbnail/7',
|
||||
type: 'individual',
|
||||
size: 1,
|
||||
uploaded_at: '2026-01-01T00:00:00Z',
|
||||
width: 4000,
|
||||
height: 3000,
|
||||
} as Photo;
|
||||
|
||||
/** Every tile in the document reports `width` CSS px. */
|
||||
function stubTileWidth(width: number) {
|
||||
Object.defineProperty(HTMLElement.prototype, 'offsetWidth', {
|
||||
configurable: true,
|
||||
get() { return width; },
|
||||
});
|
||||
}
|
||||
|
||||
function renderCard(props: Partial<React.ComponentProps<typeof PhotoCard>> = {}) {
|
||||
return render(
|
||||
<PhotoCard
|
||||
photo={PHOTO}
|
||||
isSelected={false}
|
||||
isSelectionMode={false}
|
||||
onClick={() => {}}
|
||||
onDownload={() => {}}
|
||||
onToggleSelect={() => {}}
|
||||
className="tile"
|
||||
overlayBaseClassName="overlay"
|
||||
imageProps={{ src: PHOTO.thumbnail_url!, alt: PHOTO.filename }}
|
||||
{...props}
|
||||
/>,
|
||||
);
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
seenSrcs.length = 0;
|
||||
Object.defineProperty(window, 'devicePixelRatio', { value: 3, configurable: true });
|
||||
Object.defineProperty(window, 'innerWidth', { value: 390, configurable: true });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
describe('PhotoCard tier sizing', () => {
|
||||
it('sizes from the measured tile, not the viewport', () => {
|
||||
// A 1-up Mosaic tile on a DPR-3 phone needs 1170 device px. The viewport
|
||||
// fallback assumes 2-up and would land on 600 — visibly soft.
|
||||
stubTileWidth(390);
|
||||
renderCard();
|
||||
expect(screen.getByTestId('tile')).toHaveAttribute(
|
||||
'src', '/api/gallery/x/thumbnail/7?w=900',
|
||||
);
|
||||
});
|
||||
|
||||
it('gives a dense grid the small file', () => {
|
||||
stubTileWidth(96);
|
||||
renderCard();
|
||||
// The canonical tier carries no ?w=, so these URLs stay byte-identical to
|
||||
// the ones already in browser caches.
|
||||
expect(screen.getByTestId('tile')).toHaveAttribute(
|
||||
'src', '/api/gallery/x/thumbnail/7',
|
||||
);
|
||||
});
|
||||
|
||||
it('requests exactly one URL — never a fallback then a correction', () => {
|
||||
// The measurement gate exists for this. Reading the tile from a plain
|
||||
// effect would mount the image with the viewport guess, fetch it, then
|
||||
// swap the src and fetch again — every tile in the gallery, twice.
|
||||
stubTileWidth(390);
|
||||
renderCard();
|
||||
expect(new Set(seenSrcs).size).toBe(1);
|
||||
expect(seenSrcs[0]).toContain('?w=900');
|
||||
});
|
||||
|
||||
it('measures non-lazy cards too', () => {
|
||||
// Mosaic, Masonry and Timeline do not pass `lazy`, so the observer entry
|
||||
// is never populated for them — they are precisely the layouts a
|
||||
// breakpoint guess gets most wrong.
|
||||
stubTileWidth(390);
|
||||
renderCard({ lazy: false });
|
||||
expect(screen.getByTestId('tile')).toHaveAttribute(
|
||||
'src', '/api/gallery/x/thumbnail/7?w=900',
|
||||
);
|
||||
});
|
||||
|
||||
it('leaves videos on the canonical thumbnail', () => {
|
||||
// A video's thumbnail is a poster frame, so the tier route would hand the
|
||||
// video file itself to Sharp.
|
||||
stubTileWidth(390);
|
||||
renderCard({ photo: { ...PHOTO, media_type: 'video' } as Photo });
|
||||
expect(screen.getByTestId('tile')).toHaveAttribute(
|
||||
'src', '/api/gallery/x/thumbnail/7',
|
||||
);
|
||||
});
|
||||
|
||||
it('does not put ?w= on the original-photo route', () => {
|
||||
// Layouts fall back to photo.url when thumbnail_url is null, and ?w= means
|
||||
// something else there.
|
||||
stubTileWidth(390);
|
||||
renderCard({
|
||||
photo: { ...PHOTO, thumbnail_url: undefined } as Photo,
|
||||
imageProps: { src: PHOTO.url, alt: PHOTO.filename },
|
||||
});
|
||||
expect(screen.getByTestId('tile')).toHaveAttribute('src', '/api/gallery/x/photo/7');
|
||||
});
|
||||
});
|
||||
@@ -8,7 +8,10 @@
|
||||
*/
|
||||
import { describe, it, expect, afterEach, vi } from 'vitest';
|
||||
|
||||
import { previewUrlForViewport, viewportPreviewWidth, PREVIEW_WIDTHS } from '../imageTiers';
|
||||
import {
|
||||
previewUrlForViewport, viewportPreviewWidth, thumbnailUrlForTile, tileThumbnailWidth,
|
||||
PREVIEW_WIDTHS,
|
||||
} from '../imageTiers';
|
||||
|
||||
const realWidth = window.innerWidth;
|
||||
const realDpr = window.devicePixelRatio;
|
||||
@@ -131,3 +134,107 @@ describe('previewUrlForViewport', () => {
|
||||
expect(previewUrlForViewport('/p', LANDSCAPE)).toBe('/p?w=640');
|
||||
});
|
||||
});
|
||||
|
||||
// A source big enough to fill every tier, so the tier comes from the tile
|
||||
// geometry rather than the short-edge clamp.
|
||||
const BIG = { width: 4000, height: 3000 };
|
||||
|
||||
describe('tileThumbnailWidth', () => {
|
||||
it('sizes from the tile, not the viewport', () => {
|
||||
// The bug in #1095: a 2-up phone tile is ~195 CSS px, which is 585 device
|
||||
// px at DPR 3 — the 300px thumbnail upscaled ~1.9x.
|
||||
setViewport(390, 3);
|
||||
expect(tileThumbnailWidth(BIG)).toBe(600);
|
||||
|
||||
// Same tile on a DPR-1 phone genuinely only needs 195, so it keeps the
|
||||
// small file. Sizing off the viewport alone would have shipped 600 here.
|
||||
setViewport(390, 1);
|
||||
expect(tileThumbnailWidth(BIG)).toBe(300);
|
||||
|
||||
setViewport(768, 2); // 3 up -> 256 CSS px -> 512
|
||||
expect(tileThumbnailWidth(BIG)).toBe(600);
|
||||
|
||||
setViewport(1920, 2); // 4 up -> 480 CSS px -> 960, past the top tier
|
||||
expect(tileThumbnailWidth(BIG)).toBe(900);
|
||||
});
|
||||
|
||||
it('stops at the first tier that already covers the source', () => {
|
||||
// generateThumbnail resizes withoutEnlargement, so once a tier exceeds the
|
||||
// source every larger one returns the same pixels — for a second Sharp run
|
||||
// and a second cache entry holding a byte-identical file.
|
||||
setViewport(1920, 2); // wants 900
|
||||
expect(tileThumbnailWidth({ width: 500, height: 400 })).toBe(600);
|
||||
expect(tileThumbnailWidth({ width: 260, height: 200 })).toBe(300);
|
||||
});
|
||||
|
||||
it('does not drop a tier and throw away source pixels', () => {
|
||||
// A 400px short edge fits inside no tier but 300, so clamping to the
|
||||
// largest tier it FITS IN would serve 300 and discard 100 real pixels.
|
||||
// Asking for 600 returns all 400 of them.
|
||||
setViewport(1920, 2);
|
||||
expect(tileThumbnailWidth({ width: 500, height: 400 })).not.toBe(300);
|
||||
|
||||
// And a source that comfortably clears 600 is not held back at it.
|
||||
expect(tileThumbnailWidth({ width: 800, height: 700 })).toBe(900);
|
||||
});
|
||||
|
||||
it('measures the SHORT edge, because thumbnails are square', () => {
|
||||
// A 4000x600 panorama has width to spare and can still only fill a 600
|
||||
// square. Measuring the long edge would over-ask for every panorama.
|
||||
setViewport(1920, 2);
|
||||
expect(tileThumbnailWidth({ width: 4000, height: 600 })).toBe(600);
|
||||
});
|
||||
|
||||
it('prefers the measured tile over the breakpoint fallback', () => {
|
||||
// Mosaic is 1-up on mobile where Grid is 2-up, and thumbnailScale shifts
|
||||
// every layout's column count, so the fallback is wrong for most installs
|
||||
// whenever a real measurement is available.
|
||||
setViewport(390, 3);
|
||||
expect(tileThumbnailWidth(BIG)).toBe(600); // fallback: 2 up
|
||||
expect(tileThumbnailWidth(BIG, 390)).toBe(900); // measured: 1 up
|
||||
expect(tileThumbnailWidth(BIG, 96)).toBe(300); // measured: a dense grid
|
||||
|
||||
// A zero-width measurement is a not-yet-laid-out tile, not a 0px one.
|
||||
expect(tileThumbnailWidth(BIG, 0)).toBe(600);
|
||||
});
|
||||
|
||||
it('falls back to tile geometry when dimensions are unknown', () => {
|
||||
// No guard available; the server clamps with withoutEnlargement anyway, so
|
||||
// the worst case is a tier that returns the source size.
|
||||
setViewport(390, 3);
|
||||
expect(tileThumbnailWidth()).toBe(600);
|
||||
expect(tileThumbnailWidth({ width: null, height: null })).toBe(600);
|
||||
});
|
||||
});
|
||||
|
||||
describe('thumbnailUrlForTile', () => {
|
||||
it('appends the tier the tile needs', () => {
|
||||
setViewport(390, 3);
|
||||
expect(thumbnailUrlForTile('/api/gallery/x/thumbnail/7', BIG))
|
||||
.toBe('/api/gallery/x/thumbnail/7?w=600');
|
||||
});
|
||||
|
||||
it('leaves the canonical URL byte-identical', () => {
|
||||
// No parameter for the default tier, so existing caches and ETags stay
|
||||
// valid for every client that is already holding one.
|
||||
setViewport(390, 1);
|
||||
expect(thumbnailUrlForTile('/t', BIG)).toBe('/t');
|
||||
});
|
||||
|
||||
it('preserves an existing query string', () => {
|
||||
setViewport(390, 3);
|
||||
expect(thumbnailUrlForTile('/t?wm=1', BIG)).toBe('/t?wm=1&w=600');
|
||||
});
|
||||
|
||||
it('downshifts a tier on save-data', () => {
|
||||
setViewport(390, 3, { saveData: true });
|
||||
expect(thumbnailUrlForTile('/t', BIG)).toBe('/t');
|
||||
});
|
||||
|
||||
it('returns null when there is no thumbnail to size', () => {
|
||||
// The caller is about to fall back to the original; ?w= on that route
|
||||
// means something else entirely.
|
||||
expect(thumbnailUrlForTile(null)).toBeNull();
|
||||
expect(thumbnailUrlForTile(undefined)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -21,12 +21,7 @@ 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
|
||||
// softness #1095 reports, made slightly worse. generateThumbnail resolves its
|
||||
// width from admin settings rather than an argument, so tiering it is a
|
||||
// separate change and lands separately.
|
||||
export const THUMBNAIL_WIDTHS = [300, 600, 900] as const;
|
||||
|
||||
/** Smallest tier that still covers `needed`, or the largest if none does. */
|
||||
function smallestCovering(needed: number, tiers: readonly number[]): number {
|
||||
@@ -195,3 +190,78 @@ export function adminFacePreviewUrl(
|
||||
const width = photo ? faceTierWidth(photo, cover) : FACE_CROP_WIDTH;
|
||||
return `/api/admin/photos/${eventId}/preview/${photoId}?w=${width}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Device pixels one grid tile occupies, resolved to a tier (#1095).
|
||||
*
|
||||
* `tileCssWidth` is the tile's measured rendered width, which is the only
|
||||
* honest input: column counts differ per layout (Mosaic is 1-up on mobile
|
||||
* where Grid is 2-up) and every layout shifts again with the thumbnailScale
|
||||
* theme setting, so no breakpoint table is right for all of them. When it is
|
||||
* unavailable the viewport falls back to the default grid's columns — 2 up on
|
||||
* phones, 3 on tablets, 4 on desktop — which is approximate but never worse
|
||||
* than the flat 300 it replaces.
|
||||
*
|
||||
* At the mobile default a tile is ~195 CSS px, about 585 device px on a DPR-3
|
||||
* phone, so the 300px thumbnail is upscaled ~1.9x and faces visibly mush.
|
||||
* That is the symptom #1095 reports.
|
||||
*
|
||||
* DPR is capped at 3 for the same reason as the preview tier: a DPR-10 device
|
||||
* would otherwise ask for thousands of pixels and land on the top tier for a
|
||||
* thumbnail nobody can see that much of.
|
||||
*/
|
||||
export function tileThumbnailWidth(
|
||||
photo?: { width?: number | null; height?: number | null },
|
||||
tileCssWidth?: number | null,
|
||||
): number {
|
||||
if (typeof window === 'undefined') return THUMBNAIL_WIDTHS[0];
|
||||
const dpr = Math.min(window.devicePixelRatio || 1, 3);
|
||||
const vw = window.innerWidth;
|
||||
const cssWidth = tileCssWidth && tileCssWidth > 0
|
||||
? tileCssWidth
|
||||
: vw / (vw <= 640 ? 2 : vw <= 1024 ? 3 : 4);
|
||||
const target = smallestCovering(Math.round(cssWidth * dpr), THUMBNAIL_WIDTHS);
|
||||
|
||||
// Thumbnails are square, so the source's SHORT edge is what bounds them: a
|
||||
// 4000x600 panorama can still only fill a 600 tile. Clamp to the first tier
|
||||
// that already covers the whole source — past that, withoutEnlargement means
|
||||
// every larger tier returns the same pixels, so asking buys a second Sharp
|
||||
// run and a second cache entry for a byte-identical file.
|
||||
//
|
||||
// Clamping to the largest tier the source *fits inside* would be the wrong
|
||||
// rule: a 400px source would drop to 300 and lose 100 real pixels, when
|
||||
// asking for 600 returns all 400 of them.
|
||||
const shortEdge = photo?.width && photo?.height
|
||||
? Math.min(photo.width, photo.height)
|
||||
: null;
|
||||
if (!shortEdge) return target;
|
||||
return Math.min(target, smallestCovering(shortEdge, THUMBNAIL_WIDTHS));
|
||||
}
|
||||
|
||||
/**
|
||||
* The grid thumbnail URL sized for this device (#1095).
|
||||
*
|
||||
* One URL rather than a srcset, for the same reason the lightbox picks one:
|
||||
* AuthenticatedImage fetches its `src` with the gallery bearer token and
|
||||
* renders the resulting blob. An `<img>` carrying a `w`-descriptor srcset
|
||||
* ignores `src` entirely, so that authenticated fetch would be thrown away and
|
||||
* the browser would issue its own — unauthenticated, and resolved against the
|
||||
* page origin rather than the configured API host.
|
||||
*
|
||||
* Returns the input untouched when there is nothing to size — a null
|
||||
* thumbnail_url means the caller is about to fall back to the original, and
|
||||
* adding ?w= to that URL would mean something else entirely.
|
||||
*/
|
||||
export function thumbnailUrlForTile(
|
||||
thumbnailUrl: string | null | undefined,
|
||||
photo?: { width?: number | null; height?: number | null },
|
||||
tileCssWidth?: number | null,
|
||||
): string | null {
|
||||
if (!thumbnailUrl) return null;
|
||||
const width = applyDataSaver(tileThumbnailWidth(photo, tileCssWidth), THUMBNAIL_WIDTHS);
|
||||
// The canonical tier is what the server already serves without a parameter;
|
||||
// leaving it off keeps those URLs byte-identical to today's, so existing
|
||||
// caches and ETags stay valid.
|
||||
if (width === THUMBNAIL_WIDTHS[0]) return thumbnailUrl;
|
||||
return withWidth(thumbnailUrl, width);
|
||||
}
|
||||
|
||||
@@ -19,6 +19,7 @@ import { useInView } from 'react-intersection-observer';
|
||||
import type { BaseGalleryLayoutProps } from './BaseGalleryLayout';
|
||||
import type { Photo } from '../../../types';
|
||||
import { AuthenticatedImage, PoweredBy } from '../../common';
|
||||
import { thumbnailUrlForTile } from '../imageTiers';
|
||||
import { feedbackService } from '../../../services/feedback.service';
|
||||
import { PhotoReactions } from '../PhotoReactions';
|
||||
import { useGuestIdentityOptional } from '../../../contexts/GuestIdentityContext';
|
||||
@@ -79,6 +80,15 @@ const PhotoCard: React.FC<PhotoCardProps> = ({
|
||||
threshold: 0.1,
|
||||
});
|
||||
|
||||
// Responsive tier (#1095). This layout has its own card rather than the
|
||||
// shared PhotoCard, so it needs its own call — but MasonryPhotoAlbum hands
|
||||
// the laid-out tile width straight to the render prop, so the measurement
|
||||
// the shared card has to take is simply a parameter here.
|
||||
const isVideo = photo.media_type === 'video' || photo.type === 'video';
|
||||
const tieredSrc = (!isVideo && photo.thumbnail_url
|
||||
? thumbnailUrlForTile(photo.thumbnail_url, photo, width)
|
||||
: null) || photo.thumbnail_url || photo.url;
|
||||
|
||||
const likeCount = photo.like_count ?? 0;
|
||||
const averageRating = photo.average_rating ?? 0;
|
||||
const commentCount = photo.comment_count ?? 0;
|
||||
@@ -95,7 +105,7 @@ const PhotoCard: React.FC<PhotoCardProps> = ({
|
||||
data-testid={`photo-card-${photo.id}`}
|
||||
>
|
||||
<AuthenticatedImage
|
||||
src={photo.thumbnail_url || photo.url}
|
||||
src={tieredSrc}
|
||||
alt={photo.filename}
|
||||
style={{ width, height: 'auto' }}
|
||||
className="w-full h-auto object-cover"
|
||||
|
||||
@@ -527,7 +527,21 @@ export const MasonryGalleryLayout: React.FC<BaseGalleryLayoutProps> = ({
|
||||
className="photo-grid flex gap-4"
|
||||
style={{ gap: `${gutter}px` }}
|
||||
>
|
||||
{photoColumns.map((column, columnIndex) => (
|
||||
{containerWidth === 0 ? (
|
||||
// Same gate the rows mode above already applies, for the same reason:
|
||||
// the column count starts at 3 and the greedy distribution runs with a
|
||||
// hardcoded 300px estimate until the container has been measured.
|
||||
// Mounting cards into that guess costs a full remount when it settles
|
||||
// — photos move to a different parent column, so React tears them down
|
||||
// — and since #1095 each mount picks a tier from its own width, the two
|
||||
// mounts request two DIFFERENT urls. On a 1440px desktop that was 45 of
|
||||
// 62 photos downloading twice, plus 17 left on the larger file.
|
||||
<div className="grid grid-cols-2 md:grid-cols-3 lg:grid-cols-4 gap-4">
|
||||
{photos.slice(0, 8).map((photo) => (
|
||||
<div key={photo.id} className="aspect-square bg-neutral-200 rounded-lg animate-pulse" />
|
||||
))}
|
||||
</div>
|
||||
) : photoColumns.map((column, columnIndex) => (
|
||||
<div
|
||||
key={columnIndex}
|
||||
className="flex-1 flex flex-col"
|
||||
|
||||
+115
@@ -0,0 +1,115 @@
|
||||
/**
|
||||
* Masonry columns mode must not mount cards into an unmeasured layout (#1095).
|
||||
*
|
||||
* The column count starts at 3 and the greedy distribution runs off a
|
||||
* hardcoded 300px estimate until the container has been measured. Cards
|
||||
* mounted into that guess are torn down when it settles — photos move to a
|
||||
* different parent column — and since each mount picks its tier from its own
|
||||
* width, the two mounts request two DIFFERENT urls. On a 1440px desktop that
|
||||
* was 45 of 62 photos downloading twice.
|
||||
*/
|
||||
import React from 'react';
|
||||
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
|
||||
import { render, screen } from '@testing-library/react';
|
||||
|
||||
import { MasonryGalleryLayout } from '../MasonryGalleryLayout';
|
||||
import type { Photo } from '../../../../types';
|
||||
|
||||
// Record the src every card is mounted with, so a remount at a different
|
||||
// width is visible as a second, different url for the same photo.
|
||||
const mounted: string[] = [];
|
||||
vi.mock('../../../common', () => ({
|
||||
AuthenticatedImage: ({ src, alt }: { src: string; alt?: string }) => {
|
||||
mounted.push(src);
|
||||
return <img data-testid="tile" src={src} alt={alt} />;
|
||||
},
|
||||
PoweredBy: () => null,
|
||||
}));
|
||||
|
||||
vi.mock('../../../../contexts/ThemeContext', () => ({
|
||||
useTheme: () => ({ theme: { gallerySettings: { masonryMode: 'columns' } } }),
|
||||
}));
|
||||
|
||||
vi.mock('../../../../contexts/GuestIdentityContext', () => ({
|
||||
useGuestIdentityOptional: () => null,
|
||||
}));
|
||||
|
||||
const photos: Photo[] = Array.from({ length: 6 }, (_, i) => ({
|
||||
id: i + 1,
|
||||
filename: `IMG_${i}.jpg`,
|
||||
url: `/api/gallery/x/photo/${i + 1}`,
|
||||
thumbnail_url: `/api/gallery/x/thumbnail/${i + 1}`,
|
||||
type: 'individual',
|
||||
size: 1,
|
||||
uploaded_at: '2026-01-01T00:00:00Z',
|
||||
width: 4000,
|
||||
height: 3000,
|
||||
} as Photo));
|
||||
|
||||
/**
|
||||
* jsdom reports 0 for every offsetWidth, so both the grid container and the
|
||||
* individual tiles have to be stood up. They need different values — the
|
||||
* container is what picks the column count, the tile is what picks the tier —
|
||||
* so the stub keys off the container's own class.
|
||||
*/
|
||||
function stubWidths({ container, tile }: { container: number; tile: number }) {
|
||||
Object.defineProperty(HTMLElement.prototype, 'offsetWidth', {
|
||||
configurable: true,
|
||||
get(this: HTMLElement) {
|
||||
return String(this.className).includes('photo-grid') ? container : tile;
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
const props = {
|
||||
photos,
|
||||
slug: 'x',
|
||||
onPhotoClick: () => {},
|
||||
onDownload: () => {},
|
||||
selectedPhotos: new Set<number>(),
|
||||
isSelectionMode: false,
|
||||
allowDownloads: true,
|
||||
} as never;
|
||||
|
||||
beforeEach(() => {
|
||||
mounted.length = 0;
|
||||
Object.defineProperty(window, 'devicePixelRatio', { value: 1, configurable: true });
|
||||
Object.defineProperty(window, 'innerWidth', { value: 1440, configurable: true });
|
||||
vi.stubGlobal('ResizeObserver', class {
|
||||
observe() {} unobserve() {} disconnect() {}
|
||||
});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllGlobals();
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
describe('MasonryGalleryLayout — columns mode mounts cards once', () => {
|
||||
it('shows placeholders instead of cards until the container is measured', () => {
|
||||
stubWidths({ container: 0, tile: 0 }); // never measured
|
||||
render(<MasonryGalleryLayout {...props} />);
|
||||
expect(screen.queryAllByTestId('tile')).toHaveLength(0);
|
||||
expect(mounted).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('requests exactly one url per photo once measured', () => {
|
||||
stubWidths({ container: 1440, tile: 275 });
|
||||
render(<MasonryGalleryLayout {...props} />);
|
||||
|
||||
// Six photos, six mounts — not twelve. A card mounted into the unmeasured
|
||||
// 3-column guess and remounted at the settled width would show up here as
|
||||
// a second entry for the same photo.
|
||||
expect(mounted).toHaveLength(photos.length);
|
||||
expect(new Set(mounted).size).toBe(photos.length);
|
||||
});
|
||||
|
||||
it('sizes tiles from the settled column count, not the initial 3', () => {
|
||||
// 1440 measured -> 5 columns -> ~275 CSS px tiles at DPR 1, which the
|
||||
// canonical thumbnail covers. The unmeasured 3-column guess would be
|
||||
// ~470px and would have pulled the 600 tier for every photo.
|
||||
stubWidths({ container: 1440, tile: 275 });
|
||||
render(<MasonryGalleryLayout {...props} />);
|
||||
expect(mounted.some((s) => s.includes('?w='))).toBe(false);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user