diff --git a/frontend/src/pages/admin/EventDetailsPage.tsx b/frontend/src/pages/admin/EventDetailsPage.tsx index d49992ae..711e2498 100644 --- a/frontend/src/pages/admin/EventDetailsPage.tsx +++ b/frontend/src/pages/admin/EventDetailsPage.tsx @@ -372,6 +372,12 @@ export const EventDetailsPage: React.FC = () => { const [logoUploading, setLogoUploading] = useState(false); const [currentTheme, setCurrentTheme] = useState(null); const [currentPresetName, setCurrentPresetName] = useState('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([]); // 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} diff --git a/frontend/src/services/__tests__/gallery.downloadSelectedPhotos.test.ts b/frontend/src/services/__tests__/gallery.downloadSelectedPhotos.test.ts new file mode 100644 index 00000000..b002dbe2 --- /dev/null +++ b/frontend/src/services/__tests__/gallery.downloadSelectedPhotos.test.ts @@ -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; get: ReturnType }; + +const installNavigator = (overrides: Partial<{ + userAgent: string; + platform: string; + maxTouchPoints: number; + share: ReturnType; + canShare: ReturnType; +}>) => { + 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 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); + }); +}); diff --git a/frontend/src/services/__tests__/gallery.savePhotoToDevice.test.ts b/frontend/src/services/__tests__/gallery.savePhotoToDevice.test.ts index ece86d3c..a6e777c6 100644 --- a/frontend/src/services/__tests__/gallery.savePhotoToDevice.test.ts +++ b/frontend/src/services/__tests__/gallery.savePhotoToDevice.test.ts @@ -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 , + // 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 () => { diff --git a/frontend/src/services/gallery.service.ts b/frontend/src/services/gallery.service.ts index 3cd8fee2..388b03b0 100644 --- a/frontend/src/services/gallery.service.ts +++ b/frontend/src/services/gallery.service.ts @@ -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 ~25–30 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 - // 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 { + 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 { + 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 { await api.patch(`/gallery/${slug}/photos/${photoId}/visibility`, { visibility });