From 04795219a0b66fdd1ef73748d803adfdfc0d676f Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Wed, 27 May 2026 13:16:36 +0200 Subject: [PATCH 1/3] fix(lightbox): eliminate download lag on Android by skipping the blob round-trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `savePhotoToDevice` previously buffered the full image through JS as a Blob on every platform before clicking . On cellular this added ~5s of dead air between the button press and the browser's download dialog, prompting users to re-click and produce duplicate downloads (#554 follow-up, post-#556). The blob round-trip is only required for the iOS Web Share path (`navigator.share({files})` needs File objects in hand). On Android and desktop the browser can fetch the download URL itself and show its own progress in the notification shade — instantly. So iOS keeps the existing flow; everywhere else gets a direct anchor navigation. The new `triggerDirectDownload` helper uses `api.getUri()` so the path also works in split-origin deployments (where the existing hardcoded `/api/...` pattern used by `downloadAllPhotos` would 404). Tests updated: Android / desktop / regular-Mac branches now assert that `fetchPhotoBlob` is NOT called and `triggerDirectDownload` is invoked with a `/gallery/{slug}/download/{id}` URL. iOS tests unchanged. --- .../gallery.savePhotoToDevice.test.ts | 37 +++++---- frontend/src/services/gallery.service.ts | 81 ++++++++++++------- 2 files changed, 76 insertions(+), 42 deletions(-) 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..ff393c9b 100644 --- a/frontend/src/services/gallery.service.ts +++ b/frontend/src/services/gallery.service.ts @@ -68,39 +68,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 +167,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. From d5823c79d9a187461c0126adcff7f4374cd0e8aa Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 26 May 2026 22:52:01 +0200 Subject: [PATCH 2/3] feat(lightbox): multi-photo Web Share save-to-Photos on iOS (#557) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends #531 to the selection-based bulk-download flow. On iOS with a selection at or under MAX_WEB_SHARE_FILES (25), galleryService .downloadSelectedPhotos now routes through navigator.share({ files }) so the photos land directly in Photos via the share sheet's "Save N Images" action. Above the cap, anywhere off-iOS, or on any failure, the existing server-side zip path runs unchanged. The 25-file cap is the empirically-safe ceiling: iOS Safari's share sheet starts choking beyond ~25–30 files, and every File materialises as an in-memory Blob before share() is invoked, so a 500-photo selection would buffer multiple GB on the device. trySaveMultipleToDevice exposes three outcomes: - 'shared' — share() resolved; flow ends - 'dismissed' — user cancelled (AbortError); flow ends without zip fallback so dismissal isn't silently overridden - 'fallback' — capability missing or unexpected failure; caller takes the zip path Partial shares are deliberately avoided: a single failed photo fetch collapses the whole selection back to the zip endpoint rather than sharing only the photos that resolved. All 4 grid callers (PhotoGrid, PhotoGridWithLayouts, GalleryStoryLayout, GalleryPremiumLayout) funnel through downloadSelectedPhotos, so no caller-side changes are needed. Android, desktop, Firefox, and "Download All" are untouched. Layers on top of #556 (iOS-only gating via isIOS()). Builds against the fix/android-download-web-share-554 branch. --- .../gallery.downloadSelectedPhotos.test.ts | 204 ++++++++++++++++++ frontend/src/services/gallery.service.ts | 73 ++++++- 2 files changed, 276 insertions(+), 1 deletion(-) create mode 100644 frontend/src/services/__tests__/gallery.downloadSelectedPhotos.test.ts 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/gallery.service.ts b/frontend/src/services/gallery.service.ts index ff393c9b..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 }> { @@ -221,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', }); @@ -237,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 }); From d5a37df2c41425511dc8a1f974088bebb768f0d5 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Wed, 27 May 2026 15:44:41 +0200 Subject: [PATCH 3/3] fix(events): preserve branding inheritance when saving events with null color_theme MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit API-created events (and any event whose `color_theme` is NULL) had two visible bugs in the admin edit page (#550 follow-up — PR #552 fixed the v1 POST write path, this fixes the read/save path): 1. The theme picker initialised to the hardcoded `GALLERY_THEME_PRESETS .default.config` ("Classic Grid", green) — which had nothing to do with the admin's actual branding palette, while the gallery itself was rendering with the branding theme. Confusing visual mismatch. 2. Saving the event for ANY reason (changing the date, password, etc.) wrote `color_theme = 'default'` back to the row because the save handler always emitted the picker's initial preset name. That silently replaced "inherit from branding" with the literal Classic Grid preset, so the gallery's visuals jumped. Two fixes, both in EventDetailsPage: - Add a `themeChanged` flag, defaulted false. Flip in the picker's onChange / onPresetChange / onSyncFromBranding callbacks. The save handler now only writes `updateData.color_theme` when the flag is true, so saving without touching the picker preserves NULL. - When `event.color_theme` is null and `publicSettings.theme_config` (the site branding) is available, initialise `currentTheme` from branding instead of the Classic Grid preset, with currentPresetName set to 'custom' (since inherited branding isn't a named preset). Falls back to the Classic Grid preset only when no branding theme exists either. Combined effect: opening an API-created event shows the same palette the gallery uses, and saving without changing the theme preserves the inheritance. Existing events with a stored color_theme are unaffected (themeChanged stays false → no write, just like before for the common no-change-to-theme save). --- frontend/src/pages/admin/EventDetailsPage.tsx | 31 ++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) 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}