Merge pull request #561 from the-luap/fix/android-download-latency-554

fix(lightbox+events): Android download lag, multi-photo Web Share re-land, theme branding inheritance
This commit is contained in:
Paul Nothaft
2026-05-27 15:52:44 +02:00
committed by GitHub
4 changed files with 379 additions and 47 deletions
+27 -4
View File
@@ -372,6 +372,12 @@ export const EventDetailsPage: React.FC = () => {
const [logoUploading, setLogoUploading] = useState(false);
const [currentTheme, setCurrentTheme] = useState<ThemeConfig | null>(null);
const [currentPresetName, setCurrentPresetName] = useState<string>('default');
// Tracks whether the admin actually interacted with the theme picker
// during this edit session. Prevents the save handler from writing the
// initial display state back to `events.color_theme`, which silently
// overwrote branding inheritance on events with a NULL color_theme
// (API-created events — #550 follow-up).
const [themeChanged, setThemeChanged] = useState(false);
const [cssTemplates, setCssTemplates] = useState<EnabledTemplate[]>([]);
// Fetch CSS templates when component mounts or editing starts
@@ -643,10 +649,20 @@ export const EventDetailsPage: React.FC = () => {
setCurrentPresetName('default');
}
} else {
setCurrentTheme(GALLERY_THEME_PRESETS.default.config);
setCurrentPresetName('default');
// No color_theme stored — the gallery renders with the site
// branding theme as a fallback. Mirror that here so the picker
// shows the same palette the admin sees on the gallery, rather
// than the hardcoded Classic Grid preset that has nothing to do
// with their branding (#550 follow-up). currentPresetName=custom
// because the inherited config isn't a named preset; combined
// with themeChanged=false below, saving without touching the
// picker leaves color_theme NULL and preserves inheritance.
const branding = publicSettings?.theme_config as ThemeConfig | undefined;
setCurrentTheme(branding ?? GALLERY_THEME_PRESETS.default.config);
setCurrentPresetName(branding ? 'custom' : 'default');
}
setThemeChanged(false);
setIsEditing(true);
};
@@ -765,7 +781,11 @@ export const EventDetailsPage: React.FC = () => {
if (editForm.welcome_message !== undefined && editForm.welcome_message !== null) {
updateData.welcome_message = editForm.welcome_message;
}
if (themeToSave) {
// Only persist color_theme when the admin actually interacted with
// the picker. Writing the initial display state back to the row
// silently overwrote NULL (= "inherit branding") with the picker's
// default preset on any save (#550 follow-up).
if (themeChanged && themeToSave) {
updateData.color_theme = themeToSave;
}
if (editForm.upload_category_id !== undefined) {
@@ -2210,10 +2230,12 @@ export const EventDetailsPage: React.FC = () => {
onChange={(theme) => {
setCurrentTheme(theme);
setEditForm(prev => ({ ...prev, color_theme: JSON.stringify(theme) }));
setThemeChanged(true);
}}
presetName={currentPresetName}
onPresetChange={(presetName) => {
setCurrentPresetName(presetName);
setThemeChanged(true);
if (presetName !== 'custom') {
const preset = GALLERY_THEME_PRESETS[presetName];
if (preset) {
@@ -2248,6 +2270,7 @@ export const EventDetailsPage: React.FC = () => {
setCurrentTheme(merged);
setCurrentPresetName('custom');
setEditForm(prev => ({ ...prev, color_theme: JSON.stringify(merged) }));
setThemeChanged(true);
toast.success(t('toast.brandingPaletteSynced', 'Palette synced from Branding.'));
}}
isPreviewMode={true}
@@ -0,0 +1,204 @@
/**
* Coverage for #557 — iOS Web Share path on multi-photo selection.
*
* downloadSelectedPhotos historically POSTed to /download-selected and
* triggered a zip download. On iOS with a small selection it now routes
* through navigator.share({ files }) so the photos land in Photos via
* the share sheet's "Save N Images" action. Above the file-count cap
* or anywhere off-iOS, behaviour is unchanged.
*/
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
const IOS_UA = 'Mozilla/5.0 (iPhone; CPU iPhone OS 17_2 like Mac OS X) AppleWebKit/605.1.15';
const ANDROID_UA = 'Mozilla/5.0 (Linux; Android 14; Pixel 8) AppleWebKit/537.36 Chrome/120.0.0.0';
const fetchedFor = (id: number) => ({
blob: new Blob([`photo-${id}`], { type: 'image/jpeg' }),
serverFilename: `IMG_${String(id).padStart(4, '0')}.jpg`,
});
// Mock the axios layer so the test never makes a network call. The
// real api.post resolves with { data: Blob } for the zip path; the
// shape only matters when the fallback branch is exercised.
vi.mock('../../config/api', () => ({
api: {
post: vi.fn(),
get: vi.fn(),
},
}));
let galleryService: typeof import('../gallery.service').galleryService;
let apiMock: { post: ReturnType<typeof vi.fn>; get: ReturnType<typeof vi.fn> };
const installNavigator = (overrides: Partial<{
userAgent: string;
platform: string;
maxTouchPoints: number;
share: ReturnType<typeof vi.fn>;
canShare: ReturnType<typeof vi.fn>;
}>) => {
const desc = (value: any) => ({ value, configurable: true, writable: true });
Object.defineProperties(navigator, {
userAgent: desc(overrides.userAgent ?? ''),
platform: desc(overrides.platform ?? ''),
maxTouchPoints: desc(overrides.maxTouchPoints ?? 0),
});
(navigator as any).share = overrides.share;
(navigator as any).canShare = overrides.canShare;
};
describe('galleryService.downloadSelectedPhotos — iOS Web Share path (#557)', () => {
beforeEach(async () => {
vi.resetModules();
const services = await import('../gallery.service');
galleryService = services.galleryService;
apiMock = (await import('../../config/api')).api as any;
apiMock.post.mockReset();
apiMock.post.mockResolvedValue({ data: new Blob(['zip-bytes'], { type: 'application/zip' }) });
// jsdom doesn't ship URL.createObjectURL / revokeObjectURL —
// the zip-fallback path needs both to materialise the <a> link.
(window.URL.createObjectURL as any) = vi.fn(() => 'blob:fake');
(window.URL.revokeObjectURL as any) = vi.fn();
// fetchPhotoBlob is the network-dependent helper; stub it across
// every test so we never touch the real download endpoint.
vi.spyOn(galleryService, 'fetchPhotoBlob').mockImplementation((_slug, id) =>
Promise.resolve(fetchedFor(id) as any),
);
});
afterEach(() => {
vi.restoreAllMocks();
delete (navigator as any).share;
delete (navigator as any).canShare;
});
it('routes through navigator.share on iOS with a small selection', async () => {
const share = vi.fn().mockResolvedValue(undefined);
const canShare = vi.fn().mockReturnValue(true);
installNavigator({ userAgent: IOS_UA, share, canShare });
await galleryService.downloadSelectedPhotos('wedding-2026', [1, 2, 3]);
expect(share).toHaveBeenCalledTimes(1);
const shareArg = share.mock.calls[0][0];
expect(shareArg.files).toHaveLength(3);
expect((shareArg.files[0] as File).name).toBe('IMG_0001.jpg');
// Zip endpoint must NOT be called when share succeeds.
expect(apiMock.post).not.toHaveBeenCalled();
});
it('falls back to the zip endpoint on Android, even with canShare available', async () => {
const share = vi.fn().mockResolvedValue(undefined);
const canShare = vi.fn().mockReturnValue(true);
installNavigator({ userAgent: ANDROID_UA, share, canShare });
await galleryService.downloadSelectedPhotos('wedding-2026', [1, 2, 3]);
expect(share).not.toHaveBeenCalled();
expect(apiMock.post).toHaveBeenCalledTimes(1);
expect(apiMock.post).toHaveBeenCalledWith(
'/gallery/wedding-2026/download-selected',
{ photo_ids: [1, 2, 3] },
{ responseType: 'blob' },
);
});
it('falls back to the zip endpoint above the 25-file cap', async () => {
// 26 photos: even on iOS, this exceeds MAX_WEB_SHARE_FILES so the
// Web Share path is skipped entirely (no fetchPhotoBlob calls,
// no share() call, no canShare() probe).
const share = vi.fn();
const canShare = vi.fn();
installNavigator({ userAgent: IOS_UA, share, canShare });
const ids = Array.from({ length: 26 }, (_, i) => i + 1);
await galleryService.downloadSelectedPhotos('wedding-2026', ids);
expect(galleryService.fetchPhotoBlob).not.toHaveBeenCalled();
expect(share).not.toHaveBeenCalled();
expect(apiMock.post).toHaveBeenCalledTimes(1);
});
it('takes the Web Share path at exactly the 25-file boundary', async () => {
const share = vi.fn().mockResolvedValue(undefined);
const canShare = vi.fn().mockReturnValue(true);
installNavigator({ userAgent: IOS_UA, share, canShare });
const ids = Array.from({ length: 25 }, (_, i) => i + 1);
await galleryService.downloadSelectedPhotos('wedding-2026', ids);
expect(share).toHaveBeenCalledTimes(1);
expect(apiMock.post).not.toHaveBeenCalled();
});
it('does NOT fall back to the zip endpoint when the user dismisses the share sheet (AbortError)', async () => {
const abortErr = Object.assign(new Error('user dismissed'), { name: 'AbortError' });
const share = vi.fn().mockRejectedValue(abortErr);
const canShare = vi.fn().mockReturnValue(true);
installNavigator({ userAgent: IOS_UA, share, canShare });
await galleryService.downloadSelectedPhotos('wedding-2026', [1, 2, 3]);
expect(share).toHaveBeenCalledTimes(1);
// A surprise zip in Downloads would defeat the user's deliberate
// dismissal of the share sheet.
expect(apiMock.post).not.toHaveBeenCalled();
});
it('falls back to the zip endpoint when share() rejects with a non-Abort error', async () => {
const notAllowed = Object.assign(new Error('blocked'), { name: 'NotAllowedError' });
const share = vi.fn().mockRejectedValue(notAllowed);
const canShare = vi.fn().mockReturnValue(true);
installNavigator({ userAgent: IOS_UA, share, canShare });
await galleryService.downloadSelectedPhotos('wedding-2026', [1, 2, 3]);
expect(apiMock.post).toHaveBeenCalledTimes(1);
});
it('falls back to the zip endpoint when canShare({files}) returns false (older iOS)', async () => {
const share = vi.fn();
const canShare = vi.fn().mockReturnValue(false);
installNavigator({ userAgent: IOS_UA, share, canShare });
await galleryService.downloadSelectedPhotos('wedding-2026', [1, 2, 3]);
expect(share).not.toHaveBeenCalled();
expect(apiMock.post).toHaveBeenCalledTimes(1);
});
it('falls back to the zip endpoint when any photo fetch fails (partial shares would be confusing)', async () => {
const share = vi.fn();
const canShare = vi.fn().mockReturnValue(true);
installNavigator({ userAgent: IOS_UA, share, canShare });
// Re-stub fetchPhotoBlob so the 2nd of 3 photos fails — the Promise.all
// collapse must route the whole batch to the zip endpoint rather
// than sharing only the photos that resolved.
(galleryService.fetchPhotoBlob as any).mockReset();
(galleryService.fetchPhotoBlob as any)
.mockResolvedValueOnce(fetchedFor(1))
.mockRejectedValueOnce(new Error('network'))
.mockResolvedValueOnce(fetchedFor(3));
await galleryService.downloadSelectedPhotos('wedding-2026', [1, 2, 3]);
expect(share).not.toHaveBeenCalled();
expect(apiMock.post).toHaveBeenCalledTimes(1);
});
it('falls back to the zip endpoint when the selection is empty (no Web Share invocation)', async () => {
const share = vi.fn();
const canShare = vi.fn();
installNavigator({ userAgent: IOS_UA, share, canShare });
await galleryService.downloadSelectedPhotos('wedding-2026', []);
expect(galleryService.fetchPhotoBlob).not.toHaveBeenCalled();
expect(share).not.toHaveBeenCalled();
expect(apiMock.post).toHaveBeenCalledTimes(1);
});
});
@@ -51,6 +51,7 @@ describe('galleryService.savePhotoToDevice — iOS gating (#554)', () => {
vi.spyOn(galleryService, 'fetchPhotoBlob').mockResolvedValue(fetchedBlob as any);
vi.spyOn(galleryService, 'triggerBrowserDownload').mockImplementation(() => undefined);
vi.spyOn(galleryService, 'triggerDirectDownload').mockImplementation(() => undefined);
});
afterEach(() => {
@@ -69,13 +70,18 @@ describe('galleryService.savePhotoToDevice — iOS gating (#554)', () => {
expect(share).toHaveBeenCalledTimes(1);
expect(canShare).toHaveBeenCalledWith({ files: expect.any(Array) });
expect(galleryService.triggerBrowserDownload).not.toHaveBeenCalled();
expect(galleryService.triggerDirectDownload).not.toHaveBeenCalled();
});
it('falls through to a regular download on Android even when canShare({files}) is true', async () => {
// This is the bug #554 fixes — canShare is true on Chrome Android too,
// but the Android share sheet has no "Save Image" action so the user
// sees a useless app-picker. The gating must be UA-based, not
// capability-based.
it('navigates straight to the download URL on Android (no blob round-trip)', async () => {
// #554 fix: canShare is true on Chrome Android but the share sheet
// has no "Save Image" action, so the share path is iOS-only. The
// follow-up issue (Rekoo-PS, post-#556) was that the Android
// fallback fetched the blob through JS before clicking <a download>,
// adding ~5s of dead air before the browser's download UI appeared
// and prompting users to re-click. Going straight to the download
// URL hands the fetch to the browser, which shows its own progress
// immediately — no spinner needed.
const share = vi.fn().mockResolvedValue(undefined);
const canShare = vi.fn().mockReturnValue(true);
installNavigator({ userAgent: ANDROID_UA, share, canShare });
@@ -83,21 +89,23 @@ describe('galleryService.savePhotoToDevice — iOS gating (#554)', () => {
await galleryService.savePhotoToDevice('slug', 1, 'fallback.jpg');
expect(share).not.toHaveBeenCalled();
// canShare may or may not be probed on Android; what matters is the
// share() call doesn't happen and a download is triggered instead.
expect(galleryService.triggerBrowserDownload).toHaveBeenCalledTimes(1);
expect(galleryService.triggerBrowserDownload).toHaveBeenCalledWith(
fetchedBlob.blob,
'IMG_0001.jpg',
expect(galleryService.fetchPhotoBlob).not.toHaveBeenCalled();
expect(galleryService.triggerBrowserDownload).not.toHaveBeenCalled();
expect(galleryService.triggerDirectDownload).toHaveBeenCalledTimes(1);
expect(galleryService.triggerDirectDownload).toHaveBeenCalledWith(
expect.stringMatching(/\/gallery\/slug\/download\/1$/),
'fallback.jpg',
);
});
it('falls through to a regular download on desktop Safari (no share / canShare APIs)', async () => {
it('navigates straight to the download URL on desktop Safari (no share / canShare APIs)', async () => {
installNavigator({ userAgent: DESKTOP_UA });
await galleryService.savePhotoToDevice('slug', 1, 'fallback.jpg');
expect(galleryService.triggerBrowserDownload).toHaveBeenCalledTimes(1);
expect(galleryService.fetchPhotoBlob).not.toHaveBeenCalled();
expect(galleryService.triggerBrowserDownload).not.toHaveBeenCalled();
expect(galleryService.triggerDirectDownload).toHaveBeenCalledTimes(1);
});
it('detects iPadOS 13+ (reports as MacIntel + touch) as iOS', async () => {
@@ -115,6 +123,7 @@ describe('galleryService.savePhotoToDevice — iOS gating (#554)', () => {
expect(share).toHaveBeenCalledTimes(1);
expect(galleryService.triggerBrowserDownload).not.toHaveBeenCalled();
expect(galleryService.triggerDirectDownload).not.toHaveBeenCalled();
});
it('does NOT treat a regular Mac (MacIntel + no touch) as iOS', async () => {
@@ -131,7 +140,7 @@ describe('galleryService.savePhotoToDevice — iOS gating (#554)', () => {
await galleryService.savePhotoToDevice('slug', 1, 'fallback.jpg');
expect(share).not.toHaveBeenCalled();
expect(galleryService.triggerBrowserDownload).toHaveBeenCalledTimes(1);
expect(galleryService.triggerDirectDownload).toHaveBeenCalledTimes(1);
});
it('does not fall back to download when the user dismisses the iOS share sheet (AbortError)', async () => {
+125 -29
View File
@@ -22,6 +22,14 @@ function isIOS(): boolean {
return navigator.platform === 'MacIntel' && (navigator.maxTouchPoints || 0) > 1;
}
// Hard cap on the multi-file Web Share path (#557). iOS Safari's share
// sheet starts to choke and silently fail beyond ~2530 files in
// practice; equally important, every File materialises as an in-memory
// Blob before share() is invoked, so a 500-photo @ 10 MB selection
// would buffer 5 GB on the device. Above this cap we fall through to
// the existing server-side zip flow.
const MAX_WEB_SHARE_FILES = 25;
export const galleryService = {
// Verify share token
async verifyToken(slug: string, token: string): Promise<{ valid: boolean }> {
@@ -68,39 +76,49 @@ export const galleryService = {
},
// Save single photo. iOS routes through the Web Share API so the
// share sheet's "Save Image" action lands the file in Photos;
// everywhere else (Android, desktop) uses a regular <a download>
// because their share sheets don't expose a save-to-gallery
// action — #531 originally extended this to "all mobile with
// canShare", which surfaced a useless app-picker on Android (#554).
// share sheet's "Save Image" action lands the file in Photos.
// Everywhere else (Android, desktop) navigates a hidden anchor
// straight at the download URL — the browser's native download UI
// shows up immediately and its progress lives in the notification
// shade. Buffering the blob through fetch first (the original
// path) added ~5s of dead air on cellular before any visible
// feedback, prompting users to re-click and produce duplicate
// downloads (#554 follow-up). Direct navigation eliminates the
// latency outright rather than masking it with a spinner.
async savePhotoToDevice(slug: string, photoId: number, filename: string): Promise<void> {
if (!isIOS()) {
this.triggerDirectDownload(
api.getUri({ url: `/gallery/${slug}/download/${photoId}` }),
filename,
);
return;
}
const fetched = await this.fetchPhotoBlob(slug, photoId);
const resolvedFilename = fetched.serverFilename || filename;
if (isIOS()) {
// canShare() returns false on browsers without Web Share file
// support. Probe with a representative File so the negotiation
// is accurate — `canShare({ files: [] })` returns true on some
// browsers that don't actually accept files at share() time.
const file = new File([fetched.blob], resolvedFilename, {
type: fetched.blob.type || 'image/jpeg',
});
const canShareFile =
typeof navigator !== 'undefined' &&
typeof navigator.canShare === 'function' &&
navigator.canShare({ files: [file] });
// canShare() returns false on browsers without Web Share file
// support. Probe with a representative File so the negotiation
// is accurate — `canShare({ files: [] })` returns true on some
// browsers that don't actually accept files at share() time.
const file = new File([fetched.blob], resolvedFilename, {
type: fetched.blob.type || 'image/jpeg',
});
const canShareFile =
typeof navigator !== 'undefined' &&
typeof navigator.canShare === 'function' &&
navigator.canShare({ files: [file] });
if (canShareFile) {
try {
await navigator.share({ files: [file], title: resolvedFilename });
return;
} catch (err) {
// AbortError = user dismissed the share sheet. Don't fall back —
// they made a choice. Any other failure (NotAllowedError,
// DataError, etc.) is unexpected; surface a download instead so
// the user still gets the file.
if ((err as DOMException)?.name === 'AbortError') return;
}
if (canShareFile) {
try {
await navigator.share({ files: [file], title: resolvedFilename });
return;
} catch (err) {
// AbortError = user dismissed the share sheet. Don't fall back —
// they made a choice. Any other failure (NotAllowedError,
// DataError, etc.) is unexpected; surface a download instead so
// the user still gets the file.
if ((err as DOMException)?.name === 'AbortError') return;
}
}
@@ -157,6 +175,21 @@ export const galleryService = {
window.URL.revokeObjectURL(url);
},
// Trigger a browser-native download by navigating a hidden anchor at
// the URL directly. The browser fetches the response itself (showing
// its own progress UI), so unlike triggerBrowserDownload the JS layer
// never materialises the bytes. `filename` is a hint; the server's
// Content-Disposition wins per spec, which is what carries the #493
// original-camera-filename setting through to disk.
triggerDirectDownload(href: string, filename: string): void {
const link = document.createElement('a');
link.href = href;
link.setAttribute('download', filename);
document.body.appendChild(link);
link.click();
link.remove();
},
// Download single photo — kept as the canonical name for the existing
// grid + lightbox-action callers that haven't been migrated to the
// share-aware savePhotoToDevice path yet.
@@ -196,8 +229,24 @@ export const galleryService = {
window.URL.revokeObjectURL(url);
},
// Download selected photos as ZIP
// Download selected photos. On iOS with a small selection, route
// through Web Share so the files land directly in Photos via the
// share sheet's "Save N Images" action (#557, extending #531 to the
// multi-photo case). Above the cap, or anywhere else, fall through
// to the existing server-side zip flow.
async downloadSelectedPhotos(slug: string, photoIds: number[]): Promise<void> {
if (
isIOS() &&
photoIds.length > 0 &&
photoIds.length <= MAX_WEB_SHARE_FILES
) {
const status = await this.trySaveMultipleToDevice(slug, photoIds);
// 'shared' = share() resolved; 'dismissed' = user closed the
// share sheet — both terminate the flow without touching the
// zip path. Only 'fallback' continues below.
if (status !== 'fallback') return;
}
const response = await api.post(`/gallery/${slug}/download-selected`, { photo_ids: photoIds }, {
responseType: 'blob',
});
@@ -212,6 +261,53 @@ export const galleryService = {
window.URL.revokeObjectURL(url);
},
// iOS-only Web Share path for a selection of photos.
//
// Returns:
// 'shared' — navigator.share resolved; files are now in the OS share sheet
// 'dismissed' — user cancelled the share sheet (AbortError); do NOT fall back
// 'fallback' — capability missing or unexpected failure; caller should
// use the server-side zip path instead
//
// Callers must gate by isIOS() + count <= MAX_WEB_SHARE_FILES before
// invoking this; the method does not re-check those conditions.
async trySaveMultipleToDevice(
slug: string,
photoIds: number[],
): Promise<'shared' | 'dismissed' | 'fallback'> {
let fetched: Array<{ blob: Blob; serverFilename: string | null }>;
try {
// Parallel fetch — modern browsers cap at ~6 connections per origin
// on HTTP/1.1, unlimited on HTTP/2, so 25 concurrent requests is
// safe without an explicit semaphore. A single failed fetch
// collapses the whole selection back to the zip path; partial
// shares would leave the user wondering which photos were saved.
fetched = await Promise.all(photoIds.map((id) => this.fetchPhotoBlob(slug, id)));
} catch {
return 'fallback';
}
const files = fetched.map((entry, idx) => {
const name = entry.serverFilename || `photo-${photoIds[idx]}.jpg`;
return new File([entry.blob], name, { type: entry.blob.type || 'image/jpeg' });
});
const canShareFiles =
typeof navigator !== 'undefined' &&
typeof navigator.canShare === 'function' &&
navigator.canShare({ files });
if (!canShareFiles) return 'fallback';
try {
await navigator.share({ files });
return 'shared';
} catch (err) {
if ((err as DOMException)?.name === 'AbortError') return 'dismissed';
return 'fallback';
}
},
// Toggle photo visibility (client-only)
async togglePhotoVisibility(slug: string, photoId: number, visibility: 'visible' | 'hidden'): Promise<void> {
await api.patch(`/gallery/${slug}/photos/${photoId}/visibility`, { visibility });