Skip to content

feat(routing): add OSRM as an alternate routing backend behind a provider abstraction#191

Open
jasoneplumb wants to merge 1 commit into
mainlinefrom
sweep1-189
Open

feat(routing): add OSRM as an alternate routing backend behind a provider abstraction#191
jasoneplumb wants to merge 1 commit into
mainlinefrom
sweep1-189

Conversation

@jasoneplumb

Copy link
Copy Markdown
Owner

Summary

  • Refactor src/routing.ts into a provider-agnostic dispatch layer; the active backend is selected by a top-level ROUTING_PROVIDER const (defaults to valhalla).
  • Add an OSRM client targeting the OSM-DE public backend (routing.openstreetmap.de) so we have a swappable peer when FOSSGIS Valhalla is unavailable.
  • OSRM responses are normalised to the same Route / RouteStep shape Valhalla produces — guidance.ts and the pill UI are unchanged.

Changes

  • src/routing.ts — Public types (Costing, Route, RouteStep, RouteRequest), decodePolyline6, and fetchRoute() dispatch via ROUTING_PROVIDER. Re-exports VALHALLA_URL and OSRM_BASE_URL.
  • src/routing-valhalla.ts (new) — Existing Valhalla client extracted intact (POST /route, JSON body, prose instruction strings).
  • src/routing-osrm.ts (new) — OSRM client. Selects the correct OSM-DE sub-backend (routed-car / routed-bike / routed-foot) per Costing and requests overview=full&geometries=polyline6&steps=true. Builds the unified coords[] by concatenating per-step geometries with shared-endpoint dedup, so beginShapeIndex lines up with the existing guidance state machine.
  • src/routing-osrm-instructions.ts (new) — Translates OSRM maneuver.type + modifier to Valhalla-compatible numeric type codes (so guidance.ts's icon picker keeps working) and synthesises a short English instruction ("Turn right onto Main St", "Arrive at destination", etc.).
  • src/routing.test.ts — Existing Valhalla tests retained against the extracted client; new tests cover OSRM URL construction, response parsing, error paths, geometry concatenation, maneuver-type mapping, and instruction synthesis.

Test plan

  • npm run type-check clean
  • npm run lint clean
  • npm test — 145 / 145 pass (64 in routing.test.ts)
  • npm run build succeeds; gzipped bundle 104.20 kB (well under the 105 kB cap)
  • Manual smoke: set ROUTING_PROVIDER = 'osrm', run npm run dev, drop a pin and tap "Navigate here" — verify route polyline draws, maneuver icons appear, ETA updates, and arrival fires.
  • Manual smoke: confirm default behaviour with ROUTING_PROVIDER = 'valhalla' is unchanged.

Assumptions

  • Provider selection is a build-time const for this PR — automatic failover / health detection is a separate follow-up (called out in the issue's "Out of scope").
  • OSRM maneuver.type values map cleanly to a Valhalla numeric type subset; unknown maneuvers map to 0 (default arrow icon) and synthesise a generic "Continue" instruction.
  • The OSM-DE OSRM backend layout (routed-{car,bike,foot}/route/v1/{driving,cycling,walking}/...) is what's currently deployed at routing.openstreetmap.de. If a single-host layout is preferred, osrmBackend() is the only point of change.
  • Instruction localization is explicitly out of scope per the issue.

Closes #189

…ider abstraction (#189)

Refactors src/routing.ts into a provider-agnostic dispatch layer with two
backends behind it: the existing Valhalla client and a new OSRM client
targeting the OSM-DE public backend at routing.openstreetmap.de. The
active provider is chosen by a top-level const (ROUTING_PROVIDER), still
defaulting to Valhalla — runtime failover is left to a follow-up issue.

Both providers produce the same Route / RouteStep shape so guidance.ts
and the pill UI are unchanged. OSRM maneuvers are translated into
Valhalla-compatible numeric type codes (so the existing icon picker keeps
working) and into short English instruction strings synthesized from
maneuver.type + modifier + step.name.

Closes #189

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jasoneplumb jasoneplumb added enhancement New feature or request review-requested Triggers Claude code review labels May 23, 2026
@claude

claude Bot commented May 23, 2026

Copy link
Copy Markdown

Code Review — feat(routing): OSRM backend + provider abstraction

Overall this is a well-structured refactor. The extraction is clean, tests are thorough, and the normalisation layer that maps OSRM maneuvers onto Valhalla type codes is a smart move to keep guidance.ts untouched. A few issues worth addressing before merge:


High: Circular import

routing.ts imports from routing-valhalla.ts and routing-osrm.ts, which both import back from routing.ts (for decodePolyline6 and the shared types). This is a real ESM circular dependency — not just a type cycle.

routing.ts → routing-valhalla.ts → routing.ts (decodePolyline6)
routing.ts → routing-osrm.ts    → routing.ts (decodePolyline6)

Vite/esbuild typically resolves these correctly at build time, but it's brittle — tree-shaking and init order can produce subtle runtime surprises. The clean fix is a small routing-utils.ts (or polyline.ts) that exports decodePolyline6, so the two sub-clients import from there rather than from the dispatch layer.


High: Bundle size vs. documented cap

CLAUDE.md documents the npm run size cap as 100 kB; the PR reports 104.20 kB. Either the cap was raised (and CLAUDE.md needs updating) or this PR puts the bundle over the limit. Please verify npm run size exits cleanly and update CLAUDE.md if the cap changed.


Medium: overview=full wastes bandwidth

buildOsrmUrl requests overview=full, but fetchRouteOsrm never reads route.geometry — coordinates are assembled entirely from per-step geometries. Changing to overview=false drops the redundant route-level polyline from the response, which is meaningful for long routes.

// routing-osrm.ts
const params = 'overview=false&geometries=polyline6&steps=true';

Medium: Fork type codes missing from the reference comment

osrmManeuverToValhallaType returns 23 and 24 for fork maneuvers, but the JSDoc reference table in routing-osrm-instructions.ts stops at 26=roundabout_enter and doesn't mention 23 or 24. Add 23=fork_right, 24=fork_left to keep the table accurate (and to avoid confusion when someone cross-references guidance.ts's icon picker).


Medium: OsrmStep.name should be string | null

The interface types name: string, but the OSRM HTTP API can return null for unnamed roads. The step.name ?? '' guard handles it at runtime, but the interface is misleading. Type it as string | null to make the intent explicit.


Low: Empty instruction from notification case

synthesizeOsrmInstruction returns '' for a notification maneuver with no street name:

case 'notification':
  return name ? `Continue${on}` : '';

An empty string propagated to the pill UI could render oddly. Falling back to 'Continue' (same as the default branch) is safer.


Low: fetchRouteOsrm exported but bypasses coordinate validation

Coordinate validation lives in fetchRoute() and is not repeated in fetchRouteOsrm. Since fetchRouteOsrm is a named export, a future caller that reaches for it directly (or a test that grows into one) will get a URL with NaN in it rather than a clear error. Either duplicate the guard or add a JSDoc note that it's an internal function and fetchRoute is the validated entry point.


Low: ROUTING_PROVIDER can't be changed without editing source

The PR description acknowledges this is intentional for now, so it's just a note: Vite's import.meta.env.VITE_ROUTING_PROVIDER (with a type-safe fallback) would let CI/deployment environments swap backends without source edits, whenever that becomes useful.


Nits

  • turnModifierToValhalla: slight spacing inconsistency — 'slight right':return 9; is missing the space before return that every other case has.
  • The test helper osrmOk uses routes[0].legs[0].steps but summary is passed twice — once on the leg and once on the route. The outer route.distance/duration are what fetchRouteOsrm reads for distanceM/durationS, so this is correct, but a comment would help future test authors understand the duplication.
  • The 'use lane' OSRM maneuver type was deprecated in OSRM v5.22+ in favour of the lane guidance type. Worth a comment noting it's kept for older/cached responses.

What's working well

  • The three-describe test organisation (URL construction, response parsing, maneuver mapping) is easy to navigate.
  • Shared-endpoint dedup logic and the beginShapeIndex accounting are correct and tested with a clear three-step fixture.
  • Error path coverage is solid — network failure, HTTP error, bad code, multi-leg, no-legs, abort signal — all covered for OSRM matching Valhalla.
  • guidance.ts untouched is exactly right; the maneuver normalisation layer pays for itself immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request review-requested Triggers Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(routing): add OSRM as an alternate routing backend behind a provider abstraction

1 participant