mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-21 14:36:31 +00:00
fix(map): stop real road-route fetches from dying under StrictMode
useTransportRoutes cached its AbortController in a ref that was created once and aborted on unmount. React StrictMode's dev-only mount->cleanup->remount cycle ran that abort during the *simulated* cleanup, permanently poisoning the controller before the real mount's fetch ever started — every road-routed booking (car/bus/taxi/bicycle) silently fell back to a straight line in local dev, while production builds (no StrictMode double-invoke) worked fine. Fix: create a fresh AbortController per effect run instead of a ref-cached singleton, and synchronously un-mark a job as "attempted" in that same run's cleanup if it didn't settle before the cleanup fired — so a StrictMode remount (or any other pre-completion cancellation) retries instead of being skipped forever. Verified against the real OSRM endpoint in a running dev server: all four road-routed legs on a live trip now resolve with real road geometry instead of straight lines. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -54,13 +54,13 @@ export function useTransportRoutes(reservations: Reservation[]): Map<number, [nu
|
||||
// id → waypoint signature already fetched/attempted, so an unchanged booking
|
||||
// is never re-requested even as the reservations array identity churns.
|
||||
const attemptedRef = useRef<Map<number, string>>(new Map())
|
||||
// Aborted only on unmount — a reservations change must not cancel an
|
||||
// in-flight route we still want.
|
||||
const abortRef = useRef<AbortController | null>(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<number, [nu
|
||||
for (let i = 0; i < wps.length - 1; i++) dist += haversineKm(wps[i], wps[i + 1])
|
||||
if (dist > 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<number>()
|
||||
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<number, [nu
|
||||
})
|
||||
}
|
||||
} catch {
|
||||
// Leave it unrouted — the overlay keeps the straight line.
|
||||
// A genuine failure (not this run being cancelled) also counts as
|
||||
// settled — leave it marked attempted, the overlay keeps the
|
||||
// straight line. If it WAS the cancellation that rejected this
|
||||
// fetch, cleanup already ran (synchronously, before this catch) and
|
||||
// un-marked it via `settledIds` never having gained this id in time.
|
||||
if (!cancelled) settledIds.add(job.id)
|
||||
}
|
||||
}
|
||||
})()
|
||||
return () => { cancelled = true }
|
||||
return () => {
|
||||
cancelled = true
|
||||
controller.abort()
|
||||
for (const job of jobs) {
|
||||
if (!settledIds.has(job.id)) attempted.delete(job.id)
|
||||
}
|
||||
}
|
||||
}, [reservations])
|
||||
|
||||
return routes
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user