diff --git a/client/src/hooks/useTransportRoutes.ts b/client/src/hooks/useTransportRoutes.ts index 0516431e4..afe76b763 100644 --- a/client/src/hooks/useTransportRoutes.ts +++ b/client/src/hooks/useTransportRoutes.ts @@ -54,13 +54,13 @@ export function useTransportRoutes(reservations: Reservation[]): Map>(new Map()) - // Aborted only on unmount — a reservations change must not cancel an - // in-flight route we still want. - const abortRef = useRef(null) - if (!abortRef.current) abortRef.current = new AbortController() - useEffect(() => () => abortRef.current?.abort(), []) useEffect(() => { + // Captured once: this is the same Map instance for the hook's whole + // lifetime (only ever mutated in place, never reassigned), so closing over + // it here — rather than reading `attemptedRef.current` again inside the + // cleanup below — is equivalent and keeps the cleanup lint-clean. + const attempted = attemptedRef.current const jobs: { id: number; profile: 'driving' | 'cycling'; points: { lat: number; lng: number }[] }[] = [] for (const r of reservations) { const profile = ROAD_PROFILE[r.type] @@ -71,18 +71,32 @@ export function useTransportRoutes(reservations: Reservation[]): Map MAX_ROUTE_KM) continue const key = `${profile}:${wps.map(w => `${w.lat},${w.lng}`).join('|')}` - if (attemptedRef.current.get(r.id) === key) continue - attemptedRef.current.set(r.id, key) + if (attempted.get(r.id) === key) continue + attempted.set(r.id, key) jobs.push({ id: r.id, profile, points: wps.map(w => ({ lat: w.lat, lng: w.lng })) }) } if (!jobs.length) return + // A fresh controller per effect run (never a ref-cached singleton): React + // StrictMode's dev-only mount->cleanup->remount cycle runs this effect's + // cleanup once before the real, lasting mount — a controller reused across + // that cleanup would already be permanently aborted by the time the real + // mount's fetch starts, so its request would fail instantly every time. + const controller = new AbortController() + // Ids this run's jobs have finished for (success or a genuine, non-abort + // failure) — checked synchronously in cleanup below, so it stays empty for + // a cleanup that fires before any of them settle (StrictMode's simulated + // cleanup always does, since it runs in the same tick, before any await + // resolves) and is already populated for one that fires afterward (e.g. an + // unrelated deps change once this run's fetches are long done). + const settledIds = new Set() let cancelled = false void (async () => { // Sequential to stay gentle on the shared public router. for (const job of jobs) { try { - const result = await calculateRouteWithLegs(job.points, { signal: abortRef.current?.signal, profile: job.profile }) + const result = await calculateRouteWithLegs(job.points, { signal: controller.signal, profile: job.profile }) + settledIds.add(job.id) if (cancelled) return if (result.coordinates.length >= 2) { setRoutes(prev => { @@ -92,11 +106,22 @@ export function useTransportRoutes(reservations: Reservation[]): Map { cancelled = true } + return () => { + cancelled = true + controller.abort() + for (const job of jobs) { + if (!settledIds.has(job.id)) attempted.delete(job.id) + } + } }, [reservations]) return routes diff --git a/client/tests/integration/hooks/useTransportRoutes.test.ts b/client/tests/integration/hooks/useTransportRoutes.test.ts index 787fcd8ae..357f2a8da 100644 --- a/client/tests/integration/hooks/useTransportRoutes.test.ts +++ b/client/tests/integration/hooks/useTransportRoutes.test.ts @@ -80,4 +80,35 @@ describe('useTransportRoutes (#1425 real road routes)', () => { await new Promise(r => setTimeout(r, 20)) expect(calculateRouteWithLegs).toHaveBeenCalledTimes(1) }) + + it('retries a booking whose in-flight request was cancelled before completing, instead of leaving it permanently unrouted', async () => { + // A realistic fetch-like mock: the first call only settles when its signal + // fires 'abort' — this lets the test control exactly when the in-flight + // request is cancelled, the way React StrictMode's dev-only + // mount->cleanup->remount cycle cancels the first attempt before it + // settles (both are "this render's request got cancelled" from the + // hook's point of view; only the trigger differs). + calculateRouteWithLegs.mockImplementationOnce((_points, opts) => new Promise((_resolve, reject) => { + opts.signal?.addEventListener('abort', () => reject(new DOMException('Aborted', 'AbortError'))) + })) + calculateRouteWithLegs.mockResolvedValueOnce({ coordinates: [PARIS, [48.83, 2.24], VERSAILLES], distance: 20000, duration: 1500, legs: [] }) + + const carRes = booking(9, 'car', PARIS, VERSAILLES) + const unrelated = [booking(10, 'flight', PARIS, VERSAILLES)] // no routable type -> next effect run cancels the pending request and attempts nothing new + const { rerender, result } = renderHook(({ r }) => useTransportRoutes(r), { initialProps: { r: [carRes] } }) + await waitFor(() => expect(calculateRouteWithLegs).toHaveBeenCalledTimes(1)) + + // Deps change while the request is still in flight -> the effect's cleanup + // must cancel it (same hook instance, same attemptedRef, so this is a + // deterministic stand-in for StrictMode's fake unmount). + rerender({ r: unrelated }) + await new Promise(r => setTimeout(r, 10)) + + // The same car booking comes back -> it must be retried, not silently + // skipped as "already attempted". + rerender({ r: [carRes] }) + await waitFor(() => expect(calculateRouteWithLegs).toHaveBeenCalledTimes(2)) + await waitFor(() => expect(result.current.get(9)).toBeTruthy()) + expect(result.current.get(9)).toHaveLength(3) + }) })