From 371b7da9dea281428db49124faf3943793878943 Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 18 Sep 2026 13:17:43 +0200 Subject: [PATCH 1/4] fix(roadtrip): let a drive turn round at a stop it passes through A day is routed as one request over all of its stops, and OSRM's car profile forbids a u-turn at an intermediate waypoint unless asked. When it cannot obey it refuses the whole request rather than the one leg, so a single stop on a dead end took every leg of the day with it. Gouffre de Padirac snaps onto a cave access road that goes nowhere else: each of that day's pairs routes fine alone, all five together come back 400 NoRoute, and the same five with continue_straight=false come back with their legs. The permission goes on every request that has a stop in the middle, in the browser and in the server's planning path, which had the same defect. Two-point requests keep the url they had. OSRM refuses an unknown query parameter outright rather than ignoring it, so a router that has never heard of this one is recognised by its InvalidQuery, asked again without it, and remembered per origin. --- .../components/Map/RouteCalculator.test.ts | 74 ++++++++++++++++ client/src/components/Map/RouteCalculator.ts | 84 +++++++++++++++++-- .../nest/roadtrip/roadtrip-router.service.ts | 10 ++- .../tests/unit/nest/roadtrip-router.test.ts | 25 ++++++ 4 files changed, 187 insertions(+), 6 deletions(-) diff --git a/client/src/components/Map/RouteCalculator.test.ts b/client/src/components/Map/RouteCalculator.test.ts index e7348e037..dc92b877d 100644 --- a/client/src/components/Map/RouteCalculator.test.ts +++ b/client/src/components/Map/RouteCalculator.test.ts @@ -975,3 +975,77 @@ describe('calculateAlternatives', () => { .rejects.toMatchObject({ name: 'RoutingRefusedError', status: 429 }) }) }) + +// ── turning round at a stop ──────────────────────────────────────────────────── + +/** + * Gouffre de Padirac snaps onto `Route du Puits au Salvage`, which leads nowhere else. + * OSRM's car profile forbids a u-turn at an INTERMEDIATE waypoint unless asked, and it + * refuses the ENTIRE request when it cannot obey, so a road-trip day lost every leg it + * had over one cave car park, while each of its pairs routed perfectly on its own. + */ +describe('turning round at a stop', () => { + const askedFor = async ( + call: (points: { lat: number; lng: number }[]) => Promise, + count: number, + ): Promise => { + let asked = '' + server.use(http.get(`${FOSSGIS.driving}/:coords`, ({ request }) => { + asked = request.url + return HttpResponse.json(buildOsrmRouteResponse()) + })) + await call(freshWaypoints(count)) + return asked + } + + it('FE-COMP-ROUTECALCULATOR-055: a drive that stops on the way may turn round where it stopped', async () => { + expect(await askedFor(p => calculateRoute(p), 3)).toContain('continue_straight=false') + expect(await askedFor(p => calculateRouteWithLegs(p), 3)).toContain('continue_straight=false') + expect(await askedFor(p => calculateSegments(p), 3)).toContain('continue_straight=false') + }) + + it('FE-COMP-ROUTECALCULATOR-056: two points have no middle, so the request is the one it always was', async () => { + expect(await askedFor(p => calculateRoute(p), 2)).not.toContain('continue_straight') + expect(await askedFor(p => calculateRouteWithLegs(p), 2)).not.toContain('continue_straight') + expect(await askedFor(p => calculateSegments(p), 2)).not.toContain('continue_straight') + }) + + it('FE-COMP-ROUTECALCULATOR-057: a router that has never heard of the permission is asked without it, once and then always', async () => { + // OSRM refuses an unknown parameter outright instead of ignoring it, and says which + // it is refusing: `InvalidQuery` names the query string, `NoRoute` names the road. + // Without this a router that predates the parameter would refuse every drive with a + // stop in the middle, which is a far worse fault than the one it fixes. + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: 'https://old-osrm.example.org' } })) + const asked: string[] = [] + server.use(http.get('https://old-osrm.example.org/route/v1/driving/:coords', ({ request }) => { + asked.push(request.url) + return new URL(request.url).searchParams.has('continue_straight') + ? HttpResponse.json({ code: 'InvalidQuery', message: 'Query string malformed' }, { status: 400 }) + : HttpResponse.json(buildOsrmRouteResponse()) + })) + + const first = await calculateRoute(freshWaypoints(3)) + expect(first.coordinates).toHaveLength(2) + expect(asked).toHaveLength(2) + expect(asked[1]).not.toContain('continue_straight') + + // Remembered per host, so the second drive costs one request rather than two. + await calculateRoute(freshWaypoints(3)) + expect(asked).toHaveLength(3) + expect(asked[2]).not.toContain('continue_straight') + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: '' } })) + }) + + it('FE-COMP-ROUTECALCULATOR-058: a refusal about the road is still a refusal, not a second request', async () => { + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: 'https://noroute-osrm.example.org' } })) + let calls = 0 + server.use(http.get('https://noroute-osrm.example.org/route/v1/driving/:coords', () => { + calls += 1 + return HttpResponse.json({ code: 'NoRoute', message: 'No route found between points' }, { status: 400 }) + })) + + await expect(calculateRoute(freshWaypoints(3))).rejects.toThrow() + expect(calls).toBe(1) + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: '' } })) + }) +}) diff --git a/client/src/components/Map/RouteCalculator.ts b/client/src/components/Map/RouteCalculator.ts index 3638696c5..d201ee2de 100644 --- a/client/src/components/Map/RouteCalculator.ts +++ b/client/src/components/Map/RouteCalculator.ts @@ -53,6 +53,31 @@ function routeBaseFor(profile: 'driving' | 'walking' | 'cycling'): string { return `${withoutTrailingSlashes(configured)}/route/v1/${OSRM_PROFILE_PATH[profile]}` } +/** + * Permission to turn around at a stop, for requests that have stops to turn around at. + * + * OSRM's car profile forbids a u-turn at an INTERMEDIATE waypoint unless asked otherwise, + * and a refusal is not local: the whole request comes back `400 NoRoute`, every leg of it, + * however many were fine. A stop on a dead end is enough. Gouffre de Padirac snaps onto + * `Route du Puits au Salvage`, which goes nowhere else, so a day that visits it in the + * middle lost all its legs at once while each of its pairs routed perfectly on its own. + * That is also why the fault looked like it depended on how the stops were spread across + * days: a pair on a day boundary is asked for on its own, and a request with nothing in + * the middle has nothing to refuse. + * + * Correct as well as convenient here. TREK's waypoints are places somebody stops at, not + * shape hints, and the way out of a dead end IS the way back in, and the constraint was + * buying a detour even where it did not refuse outright. + * + * Empty for two waypoints, which have no middle, so the many two-point callers + * (alternatives, the day connectors, booking geometry) keep the URL they had. + */ +const U_TURN_PARAM = '&continue_straight=false' + +function uTurnParam(waypoints: readonly Waypoint[]): string { + return waypoints.length > 2 ? U_TURN_PARAM : '' +} + /** * `fetch`, with the request counted. * @@ -79,10 +104,16 @@ async function routedFetch( let km = 0 for (let i = 1; i < waypoints.length; i++) km += haversineKm(waypoints[i - 1], waypoints[i]) const sample = { profile, surface: kind, selfHosted, waypoints: waypoints.length, km } + const asked = withoutUTurnIfRefused(url) try { - const response = await fetch(url, { signal }) + const response = await fetch(asked, { signal }) countRoute({ ...sample, failed: !response.ok }) - return response + if (response.ok || !asked.includes(U_TURN_PARAM)) return response + const plain = await droppedUTurn(asked, response) + if (!plain) return response + const second = await fetch(plain, { signal }) + countRoute({ ...sample, failed: !second.ok }) + return second } catch (err) { if (!(err instanceof DOMException && err.name === 'AbortError')) { countRoute({ ...sample, failed: true }) @@ -91,6 +122,49 @@ async function routedFetch( } } +/** + * Hosts that answered `InvalidQuery` to `continue_straight`, by origin. + * + * OSRM refuses an unknown query parameter outright rather than ignoring it, so a router + * that has never heard of this one would refuse EVERY request with a stop in the middle, + * which is a far worse fault than the one it fixes. `osrm-routed` has understood it + * since 5.6, so this is about something else answering under `routing_base_url`, and the + * first real request is the probe: there is no separate one. + * + * By origin rather than per profile, because a router either knows the parameter or does + * not; and in a Set rather than a setting, so pointing the instance at a router that does + * know it starts asking again without a reload. Same shape and the same reasoning as + * `excludeUnsupported` below. + */ +const uTurnUnsupported = new Set() + +const originOf = (url: string): string => { + try { return new URL(url).origin } catch { return url } +} + +/** The url as it should go out, with the permission dropped for a host that refused it. */ +function withoutUTurnIfRefused(url: string): string { + if (!url.includes(U_TURN_PARAM)) return url + return uTurnUnsupported.has(originOf(url)) ? url.replace(U_TURN_PARAM, '') : url +} + +/** + * The same url without the u-turn permission, once it is clear the host objects to the + * PARAMETER and not to the route. Null when the refusal was about the route itself. + * + * OSRM says which it is: `InvalidQuery` names the query string, `NoRoute` names the road. + * Reading the body is safe here because this response is about to be thrown away either + * way, and it only happens on a 400 for a request that carried the parameter. + */ +async function droppedUTurn(url: string, response: Response): Promise { + if (response.status !== 400) return null + let code: unknown + try { code = (await response.clone().json())?.code } catch { return null } + if (code !== 'InvalidQuery') return null + uTurnUnsupported.add(originOf(url)) + return url.replace(U_TURN_PARAM, '') +} + /** * A routing request the host refused, as opposed to one it answered with "no route". * @@ -153,7 +227,7 @@ export async function calculateRoute( } const coords = waypoints.map((p) => `${p.lng},${p.lat}`).join(';') - const url = `${routeBaseFor(profile)}/${coords}?overview=full&geometries=geojson&steps=false` + const url = `${routeBaseFor(profile)}/${coords}?overview=full&geometries=geojson&steps=false${uTurnParam(waypoints)}` const response = await routedFetch(url, signal, 'route', profile, waypoints) if (!response.ok) { @@ -360,7 +434,7 @@ export async function calculateSegments( if (!waypoints || waypoints.length < 2) return [] const coords = waypoints.map((p) => `${p.lng},${p.lat}`).join(';') - const url = `${routeBaseFor('driving')}/${coords}?overview=false&geometries=geojson&steps=false&annotations=distance,duration` + const url = `${routeBaseFor('driving')}/${coords}?overview=false&geometries=geojson&steps=false&annotations=distance,duration${uTurnParam(waypoints)}` const response = await routedFetch(url, signal, 'segments', 'driving', waypoints) if (!response.ok) throw new RoutingRefusedError(response.status, retryAfterMs(response)) @@ -488,7 +562,7 @@ export async function calculateRouteWithLegs( // (plugins name their own modes), which no comparison narrows to the three OSRM knows. const osrmProfile: 'driving' | 'walking' | 'cycling' = profile === 'walking' ? 'walking' : profile === 'cycling' ? 'cycling' : 'driving' - const url = `${routeBaseFor(osrmProfile)}/${coords}?overview=full&geometries=geojson&annotations=distance,duration` + const url = `${routeBaseFor(osrmProfile)}/${coords}?overview=full&geometries=geojson&annotations=distance,duration${uTurnParam(waypoints)}` const response = await routedFetch(url, signal, 'legs', osrmProfile, waypoints) if (!response.ok) throw new RoutingRefusedError(response.status, retryAfterMs(response)) diff --git a/server/src/nest/roadtrip/roadtrip-router.service.ts b/server/src/nest/roadtrip/roadtrip-router.service.ts index 99945476b..b7e7b8a55 100644 --- a/server/src/nest/roadtrip/roadtrip-router.service.ts +++ b/server/src/nest/roadtrip/roadtrip-router.service.ts @@ -244,8 +244,16 @@ export class RoadtripRouterService { const origin = base || `https://routing.openstreetmap.de/routed-${mode === 'walking' ? 'foot' : mode === 'cycling' ? 'bike' : 'car'}`; + // `continue_straight=false` because a stop is somewhere the traveller stops, not a + // shape hint. OSRM's car profile otherwise forbids a u-turn at an INTERMEDIATE + // waypoint, and it refuses the WHOLE request rather than the one leg it cannot + // obey on: a single stop on a dead end (Gouffre de Padirac snaps onto a cave + // access road that leads nowhere else) turned a whole routed day into a 400. The + // browser sends the same thing, see `uTurnParam` in client RouteCalculator.ts. + // Only with something in the middle to turn round at. + const uTurn = points.length > 2 ? '&continue_straight=false' : ''; const payload = await this.request( - `${origin}/route/v1/${path}/${points.map((p) => `${p.lng},${p.lat}`).join(';')}?overview=full&geometries=geojson&steps=false`, + `${origin}/route/v1/${path}/${points.map((p) => `${p.lng},${p.lat}`).join(';')}?overview=full&geometries=geojson&steps=false${uTurn}`, ); const parsed = osrmSchema.parse(payload); const route = parsed.routes[0]; diff --git a/server/tests/unit/nest/roadtrip-router.test.ts b/server/tests/unit/nest/roadtrip-router.test.ts index 5b317f7e3..d15afcdf1 100644 --- a/server/tests/unit/nest/roadtrip-router.test.ts +++ b/server/tests/unit/nest/roadtrip-router.test.ts @@ -105,6 +105,31 @@ describe('server Roadtrip router', () => { expect((await pending).avoidMissed).toEqual(['ferry']); expect(safeFetchAdminConfigured).toHaveBeenCalledTimes(2); }); + it('lets a drive turn round at a stop it passes through, and asks nothing extra of a single leg', async () => { + // OSRM's car profile forbids a u-turn at an INTERMEDIATE waypoint unless asked, and + // refuses the WHOLE request when it cannot obey: one stop on a dead end used to + // cost a routed day all of its legs at once. + const { router } = setup(); + await router.route(1, 1, 1, points, 'driving', []); + expect(safeFetchAdminConfigured).toHaveBeenCalledWith( + expect.stringContaining('continue_straight=false'), + expect.anything(), + ); + + vi.mocked(safeFetchAdminConfigured).mockResolvedValueOnce( + new Response(JSON.stringify({ + code: 'Ok', + routes: [{ distance: 1000, duration: 100, geometry: { coordinates: [[10, 48], [11, 49]] }, legs: [{ distance: 1000, duration: 100 }] }], + })), + ); + const pair = router.route(2, 1, 1, points.slice(0, 2), 'driving', []); + await vi.runAllTimersAsync(); + await pair; + expect(safeFetchAdminConfigured).toHaveBeenLastCalledWith( + expect.not.stringContaining('continue_straight'), + expect.anything(), + ); + }); it('does not apply car avoidance to walking routes', async () => { const route = await setup().router.route(1, 1, 1, points, 'walking', ['toll']); expect(route.avoidMissed).toEqual([]); From e9742e93224d85084b6ef3beb1dba6af802b2ad7 Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 18 Sep 2026 13:17:51 +0200 Subject: [PATCH 2/4] fix(roadtrip): a refused run asks again one pair at a time One request per day is thrifty, and the price of it is that a refusal is never local. With the usual cause gone the shape of the failure is still there, so a run the router refuses outright now asks for its legs one at a time instead of leaving the day blank. The pairs are enqueued rather than awaited, so they keep the spacing of every other request, and a rate limit is never split: answering a host that asked for less traffic with four more requests is the opposite of backing off. A refusal the host meant skips the two retries, because the same coordinates earn the same 400. Seam legs are merged under the day's own runs now rather than over them. legsByDay is replaced whole on every routing round while seamLegs only ever grows, so a pair that used to sit on a day boundary and now sits inside one day kept an answer nobody re-asks for, shaped by the vias of a stop that has moved. --- .../Roadtrip/useRoadtripRoutes.test.ts | 36 ++++ .../components/Roadtrip/useRoadtripRoutes.ts | 183 +++++++++++------- 2 files changed, 147 insertions(+), 72 deletions(-) diff --git a/client/src/components/Roadtrip/useRoadtripRoutes.test.ts b/client/src/components/Roadtrip/useRoadtripRoutes.test.ts index ed676cc62..4227dcde8 100644 --- a/client/src/components/Roadtrip/useRoadtripRoutes.test.ts +++ b/client/src/components/Roadtrip/useRoadtripRoutes.test.ts @@ -278,6 +278,42 @@ describe('useRoadtripRoutes', () => { expect(result.current.totalDistance).toBe(100000) }) + it('FE-ROADTRIP-ROUTES-028: a day the router refuses outright is asked for again one pair at a time', async () => { + const days = [day(1, 1)] + const stops: StopSpec[] = [{ id: 1, at: HAMBURG }, { id: 2, at: LUENEBURG }, { id: 3, at: BERLIN }] + // The Padirac case: one stop in the middle the router will not turn round at, and + // what comes back is a refusal of the WHOLE chain rather than of that one leg. + calculateRouteWithLegs + .mockRejectedValueOnce(new RoutingRefusedError(400, null)) + .mockResolvedValue(routed(1)) + + const { result } = renderHook(() => useRoadtripRoutes(7, days, map(1, stops))) + await waitFor(() => expect(result.current.loading).toBe(false), { timeout: 5000 }) + + // The run once, refused, then a request per pair, and no retry of the run, because + // the same coordinates earn the same refusal. + expect(calculateRouteWithLegs).toHaveBeenCalledTimes(3) + expect(calculateRouteWithLegs.mock.calls[1][0]).toHaveLength(2) + expect(calculateRouteWithLegs.mock.calls[2][0]).toHaveLength(2) + expect(result.current.days[0].legs.filter(Boolean)).toHaveLength(2) + expect(result.current.totalDistance).toBe(200000) + }) + + it('FE-ROADTRIP-ROUTES-029: a rate limit is waited out, never split into more requests', async () => { + vi.useFakeTimers() + const days = [day(1, 1)] + const stops: StopSpec[] = [{ id: 1, at: HAMBURG }, { id: 2, at: LUENEBURG }, { id: 3, at: BERLIN }] + calculateRouteWithLegs.mockRejectedValue(new RoutingRefusedError(429, null)) + + const { result } = renderHook(() => useRoadtripRoutes(7, days, map(1, stops))) + await act(async () => { await vi.advanceTimersByTimeAsync(20000) }) + + // Three attempts at the run and nothing after them: answering a host that asked for + // less traffic with two more requests would be the opposite of backing off. + expect(calculateRouteWithLegs).toHaveBeenCalledTimes(3) + expect(result.current.days[0].legs).toEqual([undefined, undefined]) + }) + it('FE-ROADTRIP-ROUTES-009: renaming a place does not re-ask the router', async () => { const days = [day(1, 1)] const stops: StopSpec[] = [{ id: 1, at: HAMBURG }, { id: 2, at: BERLIN }] diff --git a/client/src/components/Roadtrip/useRoadtripRoutes.ts b/client/src/components/Roadtrip/useRoadtripRoutes.ts index 62dcd5d0f..2637f3d94 100644 --- a/client/src/components/Roadtrip/useRoadtripRoutes.ts +++ b/client/src/components/Roadtrip/useRoadtripRoutes.ts @@ -278,6 +278,110 @@ export function useRoadtripRoutes( const collectedMisses: Record = {} const tasks: (() => Promise)[] = [] + /** + * Ask for one run, and when the router refuses the whole of it, ask for its legs + * one at a time instead. + * + * A run travels as a single request because the answer carries a leg per waypoint + * pair, which is exactly what the rail wants, and one request is one slot of a + * host that allows about one a second. What that thrift costs is that a refusal is + * never local: OSRM answers `400 NoRoute` for the entire chain when one stop in the + * MIDDLE of it sits where it may not turn around, so a day lost every leg it had + * over a single cave car park while each of its pairs routed perfectly alone. + * `uTurnParam` in RouteCalculator removes the usual cause; this removes the SHAPE of + * the failure, so a stop that really cannot be reached costs its own two legs and + * not the day around them. + * + * The split is enqueued, not awaited, so the pairs queue up behind everything else + * and keep the same spacing as any other request: a day falling back must not turn + * into a burst at the one host that refused it. + */ + const enqueueRun = (day: PlanDay, dayLegs: Record, run: RoadtripStop[], mode: string): void => { + /** The same stops, as one request per pair. A pair has nothing left to split. */ + const splitIntoPairs = (): void => { + if (run.length <= 2) return + for (let i = 0; i < run.length - 1; i++) enqueueRun(day, dayLegs, [run[i], run[i + 1]], mode) + } + tasks.push(async () => { + // Waypoints are the stops with this day's vias threaded in between them, so the + // router draws the road the traveller picked. `stopAt` remembers which waypoint + // each stop became, because the answer has a leg per waypoint PAIR and the rail + // wants one leg per stop pair. + const waypoints: { lat: number; lng: number }[] = [] + const stopAt: number[] = [] + run.forEach((stop, i) => { + stopAt.push(waypoints.length) + waypoints.push({ lat: stop.lat, lng: stop.lng }) + if (i === run.length - 1) return + // A via is filed against the day it was dropped on and the position it sits + // after IN THAT DAY. Since a chain can hold stops from an earlier day (see + // `nightSpill.ts`), the lookup has to be the stop's own day and its own + // index — this used to be the position within the chain, so on any day that + // received a night drive every via matched nothing and quietly stopped + // shaping the road. + ;(viasByDay[stop.ownerDayId ?? day.dayId] ?? []) + .filter(v => v.after_order_index === (stop.ownerIndex ?? i)) + .sort((a, b) => a.sequence - b.sequence) + .forEach(v => waypoints.push({ lat: v.lat, lng: v.lng })) + }) + + for (let attempt = 0; attempt <= RETRY_DELAYS_MS.length; attempt++) { + if (controller.signal.aborted) return + try { + const r = await calculateRouteWithLegs( + waypoints, + { signal: controller.signal, profile: mode, tripId: tripId ?? null, dayId: day.dayId, avoid }, + ) + // Where each stop ended up. stopAt[i] is that stop's waypoint index, so the + // vias threaded in between are skipped: a via is a shape handle, not a + // destination, and a dashed spur hanging off one reads as a fault. + if (r.snapped) { + const daySnaps = (collectedSnaps[day.dayId] ??= {}) + run.forEach((stop, i) => { + const s = r.snapped?.[stopAt[i]] + if (s) daySnaps[stopKey(stop)] = s + }) + } + // What was asked for against what the road turned out to be. Only + // the second engine reports it, and only when it answered: an OSRM + // fallback leaves `avoidance` absent, which is the honest reading + // of "the weighting never happened". Collected here because the + // rail plans in the browser and never sees the server's own copy + // of this field — without it the "not honoured" badge could not + // appear at all, and a motorway-free drive that is not one read as + // if the setting had held. + if (r.avoidance) { + const missed = r.avoidance.asked.filter(cls => !r.avoidance!.achieved.includes(cls)) + if (missed.length) { + collectedMisses[day.dayId] = [...new Set([...(collectedMisses[day.dayId] ?? []), ...missed])] + } + } + Object.assign(dayLegs, foldRouteRun(run, stopAt, r, mode)) + return + } catch (err) { + if (controller.signal.aborted) return + const refusal = err instanceof RoutingRefusedError ? err : null + // Almost always a rate limit on the shared routing host, or a connection + // that dropped: back off and try again. A refusal the host MEANT is the + // one thing not worth repeating: it objects to these coordinates, and + // asking twice more only spends five seconds earning the same 400. + const delay = refusal && !refusal.isRateLimit ? undefined : RETRY_DELAYS_MS[attempt] + if (delay !== undefined) { + // When the host says how long to wait, waiting less is just a second refusal. + await sleep(Math.max(delay, (refusal?.isRateLimit ? refusal.retryAfterMs : null) ?? 0), controller.signal) + continue + } + // Nothing left to try. A run of three or more stops asks again one pair at + // a time, so a stop the router will not route THROUGH costs its own two + // legs instead of every leg of the day around it. Never after a rate limit: + // the answer to a host asking for less traffic is not four more requests. + if (!refusal?.isRateLimit) splitIntoPairs() + return + } + } + }) + } + for (const day of plan) { const dayLegs: Record = {} collected[day.dayId] = dayLegs @@ -290,77 +394,7 @@ export function useRoadtripRoutes( dfMode, )) - - for (const { stops: run, mode } of runs) { - tasks.push(async () => { - // Waypoints are the stops with this day's vias threaded in between them, so the - // router draws the road the traveller picked. `stopAt` remembers which waypoint - // each stop became, because the answer has a leg per waypoint PAIR and the rail - // wants one leg per stop pair. - const waypoints: { lat: number; lng: number }[] = [] - const stopAt: number[] = [] - run.forEach((stop, i) => { - stopAt.push(waypoints.length) - waypoints.push({ lat: stop.lat, lng: stop.lng }) - if (i === run.length - 1) return - // A via is filed against the day it was dropped on and the position it sits - // after IN THAT DAY. Since a chain can hold stops from an earlier day (see - // `nightSpill.ts`), the lookup has to be the stop's own day and its own - // index — this used to be the position within the chain, so on any day that - // received a night drive every via matched nothing and quietly stopped - // shaping the road. - ;(viasByDay[stop.ownerDayId ?? day.dayId] ?? []) - .filter(v => v.after_order_index === (stop.ownerIndex ?? i)) - .sort((a, b) => a.sequence - b.sequence) - .forEach(v => waypoints.push({ lat: v.lat, lng: v.lng })) - }) - - for (let attempt = 0; attempt <= RETRY_DELAYS_MS.length; attempt++) { - if (controller.signal.aborted) return - try { - const r = await calculateRouteWithLegs( - waypoints, - { signal: controller.signal, profile: mode, tripId: tripId ?? null, dayId: day.dayId, avoid }, - ) - // Where each stop ended up. stopAt[i] is that stop's waypoint index, so the - // vias threaded in between are skipped: a via is a shape handle, not a - // destination, and a dashed spur hanging off one reads as a fault. - if (r.snapped) { - const daySnaps = (collectedSnaps[day.dayId] ??= {}) - run.forEach((stop, i) => { - const s = r.snapped?.[stopAt[i]] - if (s) daySnaps[stopKey(stop)] = s - }) - } - // What was asked for against what the road turned out to be. Only - // the second engine reports it, and only when it answered: an OSRM - // fallback leaves `avoidance` absent, which is the honest reading - // of "the weighting never happened". Collected here because the - // rail plans in the browser and never sees the server's own copy - // of this field — without it the "not honoured" badge could not - // appear at all, and a motorway-free drive that is not one read as - // if the setting had held. - if (r.avoidance) { - const missed = r.avoidance.asked.filter(cls => !r.avoidance!.achieved.includes(cls)) - if (missed.length) { - collectedMisses[day.dayId] = [...new Set([...(collectedMisses[day.dayId] ?? []), ...missed])] - } - } - Object.assign(dayLegs, foldRouteRun(run, stopAt, r, mode)) - return - } catch (err) { - // Almost always a rate limit on the shared routing host. Back off and try - // again; a run that still won't route (island hop, dead router) simply stays - // blank, and the totals say so by being partial rather than wrong. - const delay = RETRY_DELAYS_MS[attempt] - if (delay === undefined) return - // When the host says how long to wait, waiting less is just a second refusal. - const asked = err instanceof RoutingRefusedError && err.isRateLimit ? err.retryAfterMs : null - await sleep(Math.max(delay, asked ?? 0), controller.signal) - } - } - }) - } + for (const { stops: run, mode } of runs) enqueueRun(day, dayLegs, run, mode) } // One at a time, spaced out. The day sidebar can afford a small pool because it @@ -402,7 +436,12 @@ export function useRoadtripRoutes( const allLegs = useMemo(() => { const out: Record = {} for (const day of plan) Object.assign(out, legsByDay[day.dayId] ?? {}) - return { ...out, ...seamLegs } + // Seams first, this day's own runs over the top. `legsByDay` is replaced whole on + // every routing round while `seamLegs` only ever grows, so a pair that used to sit on + // a day boundary and now sits inside one day keeps an answer nobody re-asks for, and + // that older answer was shaped by the vias of a stop that has since moved. Where both + // exist the day run is the newer of the two, so it is the one to believe. + return { ...seamLegs, ...out } }, [plan, legsByDay, seamLegs]) // Which date each stop is actually reached on. Nothing is written to make it so; see From 5ef8bf31f403cfdd63891ccbb9a8be17ab7b9761 Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 18 Sep 2026 13:17:59 +0200 Subject: [PATCH 3/4] fix(roadtrip): a stop added by hand opens on the kind being looked for The button carried nothing and the form hard-wired the kind, so the categories sitting directly above it had no way through and every manual stop started life as a fuel stop with ten minutes on it. The click now carries what the corridor is looking for, dwell included. The picker is multi-select, so the first kind the panel lists wins rather than a fallback nobody switched on; with nothing selected the form falls back on its own, because a stop without a kind is a numbered destination that counts in every total. This also closes the case it was noticed on: an electric car seeds the panel to charging, so the picker said Charging over a form that said Fuel. --- .../Planner/PlaceFormModal.test.tsx | 10 +++++++ .../src/components/Planner/PlaceFormModal.tsx | 9 ++++-- .../Roadtrip/RoadtripCorridorPanel.test.tsx | 29 +++++++++++++++++++ .../Roadtrip/RoadtripCorridorPanel.tsx | 13 +++++++-- client/src/components/Roadtrip/manualStop.ts | 18 ++++++++---- .../src/components/Roadtrip/stopKinds.test.ts | 17 ++++++++++- client/src/components/Roadtrip/stopKinds.ts | 20 +++++++++++++ .../src/pages/tripPlanner/useTripPlanner.ts | 16 ++++++++-- 8 files changed, 119 insertions(+), 13 deletions(-) diff --git a/client/src/components/Planner/PlaceFormModal.test.tsx b/client/src/components/Planner/PlaceFormModal.test.tsx index acf47eb97..32ac8a7aa 100644 --- a/client/src/components/Planner/PlaceFormModal.test.tsx +++ b/client/src/components/Planner/PlaceFormModal.test.tsx @@ -1772,6 +1772,16 @@ describe('PlaceFormModal as a road trip service stop', () => { expect(screen.getAllByText('Add as a stop').length).toBeGreaterThan(0); }); + it('FE-PLANNER-PLACEFORM-085: it opens on the kind the corridor was looking for, dwell and all', () => { + // The panel seeds itself to charging on an electric car, and a form opening on a + // fuel pump under a picker that says Charging is the fault this carries the answer to. + render(); + + expect(screen.getByRole('button', { name: 'Charging' })).toHaveAttribute('aria-pressed', 'true'); + expect(screen.getByRole('button', { name: /30 min/ })).toHaveAttribute('aria-pressed', 'true'); + expect(screen.getByRole('button', { name: 'Fuel' })).toHaveAttribute('aria-pressed', 'false'); + }); + it('FE-PLANNER-PLACEFORM-076: picking a kind also picks how long that kind takes', async () => { const onSave = vi.fn().mockResolvedValue({ id: 9 }); render(); diff --git a/client/src/components/Planner/PlaceFormModal.tsx b/client/src/components/Planner/PlaceFormModal.tsx index 901547203..eb0979136 100644 --- a/client/src/components/Planner/PlaceFormModal.tsx +++ b/client/src/components/Planner/PlaceFormModal.tsx @@ -254,10 +254,15 @@ function usePlaceFormModal(props: PlaceFormModalProps) { // path exists to avoid. Read out of the closure rather than watched, because the // mode is fixed for the life of one opening and a rebuilt drive must not reset a // half-filled form. + // What the corridor panel was looking for when the button was pressed, because + // that is the traveller's own answer to what they are adding; only a panel with + // nothing selected falls back to the constant. The dwell follows the kind, the + // same way picking one by hand moves it. + const kind = serviceStop.defaultKind ?? DEFAULT_SERVICE_KIND setForm({ ...DEFAULT_FORM, - stop_type: DEFAULT_SERVICE_KIND, - duration_minutes: STOP_KIND_BY_KEY[DEFAULT_SERVICE_KIND].defaultMinutes, + stop_type: kind, + duration_minutes: STOP_KIND_BY_KEY[kind].defaultMinutes, }) } else { setForm(DEFAULT_FORM) diff --git a/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx b/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx index fdf90b226..3129b6b45 100644 --- a/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx +++ b/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx @@ -441,6 +441,35 @@ describe('RoadtripCorridorPanel', () => { // nothing is added until a place has been found and saved in there. fireEvent.click(manual) expect(onAddManual).toHaveBeenCalledTimes(1) + // The kind, never the MouseEvent: the categories above the button are the answer to + // what is being added, and a form that ignored them opened every manual stop on fuel. + expect(onAddManual).toHaveBeenCalledWith('fuel') + }) + + it('FE-ROADTRIP-PANEL-035: the manual add opens on what the panel is looking for', () => { + const open = (categories: string[]): unknown => { + const onAddManual = vi.fn() + const { unmount } = wrap( + , + ) + fireEvent.click(searchRow()[1]) + unmount() + return onAddManual.mock.calls[0][0] + } + + // An electric car seeds the panel to charging, and this is the case the fault was + // reported on: the panel said Charging and the form opened on Fuel. + expect(open(['charging'])).toBe('charging') + expect(open(['restaurant'])).toBe('restaurant') + // Multi-select: the first the panel LISTS wins, rather than a kind nobody switched on. + expect(open(['sights', 'rest_area'])).toBe('rest_area') + // Nothing selected is no answer, and the form falls back on its own. + expect(open([])).toBeNull() }) it('FE-ROADTRIP-PANEL-026: with no drive on the trip there is nowhere to add one by hand', () => { diff --git a/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx b/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx index 505b5f82c..850044776 100644 --- a/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx +++ b/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx @@ -13,9 +13,10 @@ import CustomSelect from '../shared/CustomSelect' import RoadtripCategoryPicker from './RoadtripCategoryPicker' import { serviceColor } from './roadtripModel' import { alongLabel, offRouteLabel } from './corridorSearchModel' -import { CORRIDOR_CATEGORY_BY_KEY } from './stopKinds' +import { CORRIDOR_CATEGORY_BY_KEY, manualStopKindFor } from './stopKinds' import { FS } from './typeScale' import { CORRIDOR_CATEGORY_KEYS, CORRIDOR_SECTION_KM, CORRIDOR_WIDTHS_KM, type RoadtripCorridor } from './useRoadtripCorridor' +import type { RoadtripStopType } from '@trek/shared' import type { CorridorPoi } from './useCorridorPois' import type { RoadtripRoutes } from './useRoadtripRoutes' @@ -38,8 +39,12 @@ interface RoadtripCorridorPanelProps { /** * Opens the place form for a stop the search never found. Given only to somebody who * may add places, which is what keeps the button off a reader's panel. + * + * Carries the kind the panel is looking for, so the form opens on it rather than on a + * constant: the categories above the button are the traveller's own answer to "what am + * I adding", and ignoring them made every manual stop start life as a fuel stop. */ - onAddManual?: () => void + onAddManual?: (kind: RoadtripStopType | null) => void } /** @@ -476,7 +481,9 @@ export default function RoadtripCorridorPanel({ {onAddPoi && onAddManual && corridor.day ? (