From 94674e757248b36022faeebf9630d586c8295a36 Mon Sep 17 00:00:00 2001 From: Maurice Date: Tue, 20 Oct 2026 20:43:28 +0200 Subject: [PATCH] fix(tours): open the GPX share sheet inside the tap Sharing a tour fetched the GPX file first and only then called navigator.share, so the tap that asked had often run out by then. The retry path kept that file for one more tap only, and a tap while a sheet was still open made the browser throw InvalidStateError, which read as a failed share. The share button now fetches the file ahead where sharing is offered (not while the planner has unsaved changes), and every tap hands a fresh File built from the kept bytes to the sheet before anything is awaited. A file a tap had to fetch is kept for the next tap. A tap while a sheet is open is let be, and InvalidStateError and a closed sheet stay quiet; only a real failure still says the download is left. --- .../components/Tours/TourGpxExportButton.tsx | 4 +- .../Tours/useTourGpxExport.test.tsx | 86 ++++++++++++++++++- .../src/components/Tours/useTourGpxExport.ts | 76 +++++++++++++--- client/src/utils/fileShare.test.ts | 4 +- client/src/utils/fileShare.ts | 7 +- 5 files changed, 158 insertions(+), 19 deletions(-) diff --git a/client/src/components/Tours/TourGpxExportButton.tsx b/client/src/components/Tours/TourGpxExportButton.tsx index d62c0e985..30dc4c185 100644 --- a/client/src/components/Tours/TourGpxExportButton.tsx +++ b/client/src/components/Tours/TourGpxExportButton.tsx @@ -13,7 +13,7 @@ import { useTourGpxExport, type TourGpxExportTarget } from './useTourGpxExport'; */ export function TourGpxExportButton({ target }: { target: TourGpxExportTarget }) { const { t } = useTranslation(); - const { exportGpx, shareGpx, canShare, exporting } = useTourGpxExport(); + const { exportGpx, shareGpx, canShare, exporting } = useTourGpxExport(target); return ( <> @@ -54,7 +54,7 @@ export function TourGpxExportButton({ target }: { target: TourGpxExportTarget }) */ export function TourGpxExportBarButton({ target, unsaved }: { target: TourGpxExportTarget; unsaved: boolean }) { const { t } = useTranslation(); - const { exportGpx, shareGpx, canShare, exporting } = useTourGpxExport(); + const { exportGpx, shareGpx, canShare, exporting } = useTourGpxExport(unsaved ? null : target); return ( <> { await act(() => result.current.shareGpx(target)); expect(toast.error).toHaveBeenLastCalledWith('tours.gpxExport.shareFailed'); - share.shareFile.mockResolvedValueOnce('needs-gesture').mockResolvedValueOnce('needs-gesture'); - await act(() => result.current.shareGpx(target)); + share.shareFile.mockResolvedValueOnce('needs-gesture'); await act(() => result.current.shareGpx(target)); + expect(repo.exportGpx).toHaveBeenCalledTimes(1); + expect(toast.info).not.toHaveBeenCalled(); expect(toast.error).toHaveBeenCalledTimes(2); }); + it('a second tap shares again from the kept file, as a fresh file, and says nothing', async () => { + share.shareFile.mockResolvedValue('shared'); + const { result } = renderHook(() => useTourGpxExport()); + await act(() => result.current.shareGpx(target)); + await act(() => result.current.shareGpx(target)); + expect(repo.exportGpx).toHaveBeenCalledTimes(1); + expect(share.shareFile).toHaveBeenCalledTimes(2); + const [first, second] = share.shareFile.mock.calls.map((call) => call[0] as File); + expect(second).not.toBe(first); + expect(second).toEqual(gpxFile('Kramer-über.gpx')); + expect(toast.info).not.toHaveBeenCalled(); + expect(toast.error).not.toHaveBeenCalled(); + }); + + it('fetches the file ahead, so the tap opens the sheet before anything is awaited', async () => { + share.canShareFile.mockReturnValue(true); + share.shareFile.mockResolvedValue('shared'); + const { result } = renderHook(() => useTourGpxExport(target)); + await act(async () => undefined); + expect(repo.exportGpx).toHaveBeenCalledWith(1, 7, expect.any(AbortSignal)); + act(() => { + void result.current.shareGpx(target); + }); + // Called in the same turn as the tap, not after a network round trip. + expect(share.shareFile).toHaveBeenCalledWith(gpxFile('Kramer-über.gpx'), 'Kramer'); + expect(repo.exportGpx).toHaveBeenCalledTimes(1); + await act(async () => undefined); + expect(toast.error).not.toHaveBeenCalled(); + }); + + it('lets a tap be while the sheet is still open, and a busy browser is no failure', async () => { + let close: (value: unknown) => void = () => undefined; + share.shareFile.mockImplementationOnce(() => new Promise((resolve) => (close = resolve))); + const { result } = renderHook(() => useTourGpxExport()); + await act(async () => { + void result.current.shareGpx(target); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + expect(share.shareFile).toHaveBeenCalledTimes(1); + await act(() => result.current.shareGpx(target)); + expect(share.shareFile).toHaveBeenCalledTimes(1); + await act(async () => close('cancelled')); + + share.shareFile.mockResolvedValueOnce('busy'); + await act(() => result.current.shareGpx(target)); + expect(share.shareFile).toHaveBeenCalledTimes(2); + expect(toast.info).not.toHaveBeenCalled(); + expect(toast.error).not.toHaveBeenCalled(); + }); + + it('a failed fetch ahead is quiet, and the tap fetches itself and says why', async () => { + share.canShareFile.mockReturnValue(true); + repo.exportGpx.mockRejectedValueOnce(new Error('network')).mockRejectedValueOnce(refused(422)); + const { result } = renderHook(() => useTourGpxExport(target)); + await act(async () => undefined); + expect(toast.error).not.toHaveBeenCalled(); + await act(() => result.current.shareGpx(target)); + expect(repo.exportGpx).toHaveBeenCalledTimes(2); + expect(share.shareFile).not.toHaveBeenCalled(); + expect(toast.error).toHaveBeenCalledWith('tours.gpxExport.noRoute'); + }); + + it('fetches nothing ahead without the share, and drops the kept file when the tour changes', async () => { + const { unmount } = renderHook(() => useTourGpxExport(target)); + await act(async () => undefined); + expect(repo.exportGpx).not.toHaveBeenCalled(); + unmount(); + + share.canShareFile.mockReturnValue(true); + share.shareFile.mockResolvedValue('shared'); + const { result, rerender } = renderHook(({ prefetch }) => useTourGpxExport(prefetch), { + initialProps: { prefetch: target as typeof target | null }, + }); + await act(async () => undefined); + rerender({ prefetch: null }); + const signal = repo.exportGpx.mock.calls[0][2] as AbortSignal; + expect(signal.aborted).toBe(true); + await act(() => result.current.shareGpx(target)); + expect(repo.exportGpx).toHaveBeenCalledTimes(2); + }); + it('a kept file belongs to its tour: another tour is fetched afresh', async () => { share.shareFile.mockResolvedValueOnce('needs-gesture').mockResolvedValueOnce('shared'); const { result } = renderHook(() => useTourGpxExport()); diff --git a/client/src/components/Tours/useTourGpxExport.ts b/client/src/components/Tours/useTourGpxExport.ts index ec5548f8b..55c5c16c2 100644 --- a/client/src/components/Tours/useTourGpxExport.ts +++ b/client/src/components/Tours/useTourGpxExport.ts @@ -3,11 +3,21 @@ import { useCallback, useEffect, useRef, useState } from 'react'; import { useTranslation } from '../../i18n'; import { tourRepo } from '../../repo/tourRepo'; import { downloadBlob } from '../../utils/fileDownload'; -import { canShareFile, shareFile } from '../../utils/fileShare'; +import { canShareFile, shareFile, type FileShareOutcome } from '../../utils/fileShare'; import { useToast } from '../shared/Toast'; const GPX_TYPE = 'application/gpx+xml'; +/** A tour's GPX file kept for the next share, as the bytes and the name the server gave them. */ +interface PreparedFile { + placeId: number; + blob: Blob; + name: string; +} + +/** The file's name when the server gave none: the tour's, or plain "tour". */ +const fallbackName = (name: string) => `${name.trim() || 'tour'}.gpx`; + /** What a tour's GPX export needs to know about the tour. */ export interface TourGpxExportTarget { tripId: number | string; @@ -22,18 +32,52 @@ export interface TourGpxExportTarget { * so an unsaved edit in the planner is not in it. One export at a time per * caller; a dialog closed mid-download cancels it. Nothing about the tour * changes either way. + * + * The share sheet only opens from inside the tap that asks for it, and a tap + * that first waits on the network has often run out by the time the file is + * there. So where sharing is offered, the file of `prefetch` (the tour on + * screen, null while it has unsaved changes) is fetched ahead, quietly, and a + * tap hands it to the sheet before anything is awaited. A file a tap had to + * fetch is kept for the next one the same way. */ -export function useTourGpxExport() { +export function useTourGpxExport(prefetch: TourGpxExportTarget | null = null) { const { t } = useTranslation(); const toast = useToast(); const [exportingId, setExportingId] = useState(null); const abortRef = useRef(null); - // A file fetched for a share the browser then refused for want of a fresh tap, kept for the next one. - const readyRef = useRef<{ placeId: number; file: File } | null>(null); + // The tour's file at hand for a share, so the tap opens the sheet without waiting on the network. + const preparedRef = useRef(null); + // An open share sheet: a second tap while it is open would only make the browser refuse. + const sharingRef = useRef(false); const [canShare] = useState(() => canShareFile('tour.gpx', GPX_TYPE)); useEffect(() => () => abortRef.current?.abort(), []); + const prefetchTripId = prefetch?.tripId ?? null; + const prefetchPlaceId = prefetch?.placeId ?? null; + const prefetchName = prefetch?.name ?? ''; + useEffect(() => { + if (!canShare || prefetchTripId === null || prefetchPlaceId === null) return; + const controller = new AbortController(); + tourRepo.exportGpx(prefetchTripId, prefetchPlaceId, controller.signal).then( + (result) => { + if (controller.signal.aborted || !('blob' in result)) return; + preparedRef.current = { + placeId: prefetchPlaceId, + blob: result.blob, + name: result.filename ?? fallbackName(prefetchName), + }; + }, + // Quiet on purpose: a tap then fetches the file itself and says why it could not. + () => undefined + ); + return () => { + controller.abort(); + // Another tour, or the same one with changes the kept file does not hold. + preparedRef.current = null; + }; + }, [canShare, prefetchTripId, prefetchPlaceId, prefetchName]); + /** The tour's file from the server, or null when there is none to hand over, said in a toast. */ const fetchFile = useCallback( async ({ tripId, placeId, name }: TourGpxExportTarget): Promise => { @@ -47,7 +91,7 @@ export function useTourGpxExport() { toast.info(t('tours.gpxExport.offline')); return null; } - return new File([result.blob], result.filename ?? `${name.trim() || 'tour'}.gpx`, { type: GPX_TYPE }); + return new File([result.blob], result.filename ?? fallbackName(name), { type: GPX_TYPE }); } catch (error: unknown) { if (controller.signal.aborted) return null; const noRoute = isAxiosError(error) && error.response?.status === 422; @@ -71,14 +115,22 @@ export function useTourGpxExport() { const shareGpx = useCallback( async (target: TourGpxExportTarget) => { - const ready = readyRef.current?.placeId === target.placeId ? readyRef.current.file : null; - readyRef.current = null; - // A file at hand goes to the sheet before anything is awaited, inside the tap that asked. - const file = ready ?? (await fetchFile(target)); + if (sharingRef.current) return; + const prepared = preparedRef.current?.placeId === target.placeId ? preparedRef.current : null; + // A fresh File for every sheet, so no share is handed one an earlier sheet still holds. + // It goes to the sheet before anything is awaited, inside the tap that asked. + const file = prepared ? new File([prepared.blob], prepared.name, { type: GPX_TYPE }) : await fetchFile(target); if (!file) return; - const outcome = await shareFile(file, target.name); - if (outcome === 'needs-gesture' && !ready) { - readyRef.current = { placeId: target.placeId, file }; + if (!prepared) preparedRef.current = { placeId: target.placeId, blob: file, name: file.name }; + sharingRef.current = true; + let outcome: FileShareOutcome; + try { + outcome = await shareFile(file, target.name); + } finally { + sharingRef.current = false; + } + // A closed sheet is the user's choice and a sheet still open is no failure: neither says anything. + if (outcome === 'needs-gesture' && !prepared) { toast.info(t('tours.gpxExport.shareReady')); } else if (outcome === 'failed' || outcome === 'needs-gesture') { toast.error(t('tours.gpxExport.shareFailed')); diff --git a/client/src/utils/fileShare.test.ts b/client/src/utils/fileShare.test.ts index 481c866c4..ab57fd918 100644 --- a/client/src/utils/fileShare.test.ts +++ b/client/src/utils/fileShare.test.ts @@ -41,13 +41,15 @@ describe('shareFile (IO-01)', () => { expect(share).toHaveBeenCalledWith({ files: [file], title: 'Kramer' }); }); - it('tells a closed sheet, a refused tap and any other failure apart', async () => { + it('tells a closed sheet, a refused tap, an open sheet and any other failure apart', async () => { const share = vi.fn(); stubShare(share, () => true); share.mockRejectedValueOnce(new DOMException('closed', 'AbortError')); await expect(shareFile(gpx(), 'Kramer')).resolves.toBe('cancelled'); share.mockRejectedValueOnce(new DOMException('no activation', 'NotAllowedError')); await expect(shareFile(gpx(), 'Kramer')).resolves.toBe('needs-gesture'); + share.mockRejectedValueOnce(new DOMException('earlier share still open', 'InvalidStateError')); + await expect(shareFile(gpx(), 'Kramer')).resolves.toBe('busy'); share.mockRejectedValueOnce(new DOMException('bad data', 'DataError')); await expect(shareFile(gpx(), 'Kramer')).resolves.toBe('failed'); share.mockRejectedValueOnce(new Error('odd')); diff --git a/client/src/utils/fileShare.ts b/client/src/utils/fileShare.ts index bde9812e3..8819f2c53 100644 --- a/client/src/utils/fileShare.ts +++ b/client/src/utils/fileShare.ts @@ -20,9 +20,11 @@ export function canShareFile(name: string, type: string): boolean { /** * How a share ended: `cancelled` when the sheet was closed without a target, * `needs-gesture` when the browser refused because the tap that asked lies too - * far back (Safari, after the file had to be fetched first), `failed` otherwise. + * far back (Safari, after the file had to be fetched first), `busy` when an + * earlier sheet is still open (the browser's InvalidStateError), `failed` + * otherwise. */ -export type FileShareOutcome = 'shared' | 'cancelled' | 'needs-gesture' | 'failed'; +export type FileShareOutcome = 'shared' | 'cancelled' | 'needs-gesture' | 'busy' | 'failed'; /** Opens the share sheet with the file; call it straight from the tap that asks for it. */ export async function shareFile(file: File, title: string): Promise { @@ -32,6 +34,7 @@ export async function shareFile(file: File, title: string): Promise