Fixing Path Between Destinations

[KesarSidhu]

Summary

• Fixes Results map polylines so routes follow the road network instead of drawing straight lines between stops.
• Addresses DirectionsService failures caused by parallel requests, fragile response handling, and a second effect that overwrote road paths with straight-line fallbacks.
• Also includes prior results fixes on this branch: stop marker anchoring at all zoom levels and removal of silent mock-route fallback on /results.

Motivation

• After optimize, route lines on /results looked like direct stop-to-stop segments, not drivable paths — misleading for drivers/dispatchers even though lat/lng data was correct.
• RoutePolylinesOverlay already called Google DirectionsService, but in practice most routes hit the straight-line fallback (drawFallback).
• Mock data often appeared fine (few stops, one route); real optimize data with multiple vehicles was more likely to fail due to rate limits and effect timing.

Changes

Frontend (app/ui)

• Map.tsx — road-following polylines
  • Add requestDrivingDirections() with explicit DirectionsStatus.OK check (callback-based, not fire-and-forget await).
  • Add extractRoadPath() — uses overview_path, falls back to leg step paths when overview is empty.
  • Fetch directions sequentially (one route at a time) to reduce OVER_QUERY_LIMIT failures.
  • Stop second effect from overwriting cached road paths with straight setPath(committed) on cache miss.
  • Stabilize polyline effect deps with routesPathKey so note/distance edits don’t tear down and redraw directions.
  • Keep existing >25-waypoint straight-line fallback (Google API limit; out of scope to split legs).
• Map.tsx — marker anchoring (prior commit on branch)
  • Remove duplicate Advanced Marker translate; always set legacy Marker scaledSize + bottom-center anchor.
• page.tsx — data loading (prior commit on branch)
  • Remove ?mock=1 and automatic mock fallback; show error overlay when no/corrupt sessionStorage data.
• .env.example
  • Document that Maps JavaScript API and Directions API must be enabled for NEXT_PUBLIC_GOOGLE_MAPS_KEY.

Backend / mobile app / infra

• No backend, mobile app, or infrastructure changes in this PR.

Validation

Frontend

• npm --prefix app/ui run lint
• npm --prefix app/ui run format:check
• npm --prefix app/ui run typecheck
• npm --prefix app/ui run test
• npm --prefix app/ui run build
• npm --prefix app/ui run test:e2e
• npm --prefix app/mobile run lint
• npm --prefix app/mobile run typecheck

Backend

• cmake --preset dev
• .github/scripts/check-backend-static.sh build/dev
• cmake --build --preset dev --parallel
• ctest --preset dev --output-on-failure --no-tests=error -LE 'e2e|docker'
• docker compose -f deploy/compose/docker-compose.arm64.yml --env-file deploy/env/http-server.arm64.env config

Manual checks

• Run optimize from /edit → open /results: polylines follow roads/turns between depot and stops.
• Zoom in/out: stop markers stay pinned to their lat/lng (no geographic drift).
• Multiple vehicles/routes: each route gets a road-following polyline; console should not spam DirectionsService failed.
• Direct /results visit without optimize: error overlay (no mock routes).
• Route with >25 intermediate stops: straight-line fallback still applies (expected, documented out of scope).
• Confirm Google Cloud project has Directions API enabled for the Maps key.

Risk

• Low–medium (map rendering): Directions logic refactor touches polyline lifecycle only; stop coordinates and solver output unchanged.
• Directions API dependency: Road polylines require Directions API enabled and within quota; failures still fall back to straight lines per route.
• Sequential fetching: Multiple routes load polylines one-by-one (slightly slower than parallel, but more reliable).
• UX: Removing mock fallback means /results requires prior optimize or shows error overlay.

Rollout and Recovery

• Ship as frontend-only deploy of app/ui; no migrations or feature flags.
• Ensure production Maps API key has Directions API enabled before deploy.
• Roll back by reverting this PR; polylines return to prior behavior (straight-line fallback more often).
• If only road-path logic regresses, revert Map.tsx RoutePolylinesOverlay changes while keeping marker/data-loading fixes.

High-Signal PR Checklist

• The summary states the user-visible or operational outcome, not just file names.
• The motivation explains why this change is needed now.
• The change list separates frontend, backend, infrastructure, and documentation work where applicable.
• Validation includes exact commands run, relevant output, and any checks intentionally skipped.
• Risks, migrations, feature flags, and rollback steps are called out when relevant.
• Screenshots or request/response examples are included for UI and API behavior changes.

Closes #195

[markboenigk]

Great job! All CI checks are green and the logic looks solid. Same situation as #201 — there's a merge conflict, so could you rebase onto main and force-push? Happy to merge once that's resolved.

[markboenigk]

Good refactor overall — the sequential fetch, extractRoadPath fallback, and marker anchor fix are all solid improvements. A few issues worth addressing before merge:

1. Cache-miss after pin-commit leaves polyline stale (regression)

The PR removes the else { poly.setPath(committed); } branch from the pin-move sync effect. When a user commits a pin drag and the Directions cache is cold for the new coordinates, nothing sets the polyline path — it stays at whatever shape it had from the drag preview until the async fetch finishes. The straight-line snap was the intended intermediate state. Suggest keeping it:

if (cached && cached.path.length >= 2) {
  poly.setPath(cached.path);
} else {
  poly.setPath(committed); // straight-line until async fetch fills the cache
}

2. Hydration mismatch: server renders no error modal, client does

readInitialRoutes now returns {error: "No optimized routes found…"} when sessionStorage is empty. On the server the typeof window === "undefined" guard fires first and returns {error: null}. Next.js hydrates the server HTML (no modal) against the client's initial state (modal present), producing a hydration mismatch. One fix is to show the error only after a useEffect confirms we're on the client with no data — same pattern as the ?mock=1 branch.

3. Blank-map flash when savePendingPinMove fires

savePendingPinMove batches setRoutes + setPendingPinMove(null) into one render, so both effects fire together. Effect 1 (directions) clears polylinesByVehicleRef.current = {} synchronously before launching its async IIFE. Effect 2 (pin-move sync) then reads the now-empty ref, finds no polylines, and silently skips the snap-back. The map shows nothing until the async fetch resolves. This was probably unintentional — the effect ordering interplay here is subtle.

4. Minor: extractRoadPath step fallback produces duplicate vertices

Adjacent steps share an endpoint (step[N].path[-1] === step[N+1].path[0]), so concatenating them raw duplicates the boundary point at every step join. Easy fix: skip the first point of each subsequent step's path.

for (const [i, step] of (leg.steps ?? []).entries()) {
  if (step.path?.length) {
    path.push(...(i === 0 ? step.path : step.path.slice(1)));
  }
}

[markboenigk]

Thanks for the sequential-fetch refactor and the routesPathKey dep stabilization — those are the right fixes. A few issues in the new code before this is ready to merge:

[Bug — high] Polyline ref leak from effect ordering

Effect 1 clears polylinesByVehicleRef.current = {} synchronously and then starts the async loop. Effect 2 runs immediately after (same React flush), finds the ref empty, creates a new google.maps.Polyline({map, ...}) for every route, and writes them into the ref. When the async loop later creates road polylines for the same vehicles it overwrites those ref slots — but never calls setMap(null) on effect 2's polylines first. Those polylines stay on the map indefinitely. After a few route changes the canvas accumulates stacked duplicate lines.

Fix: in effect 2's !poly branch, don't create a new Polyline — continue (or return) as the old code did. The async loop in effect 1 owns polyline creation; effect 2 should only update existing polylines.

[Bug — medium] extractRoadPath duplicates waypoint coords at leg boundaries

The fallback loop uses for (const [i, step] of leg.steps.entries()) where i resets to 0 for the first step of every leg. The guard i === 0 ? step.path : step.path.slice(1) deduplicates within a single leg but not between legs — the last coordinate of leg N equals the first coordinate of leg N+1's first step, and since i === 0 it's pushed again. Every multi-waypoint route that hits the fallback gets a duplicate LatLng at each intermediate stop, which produces a visible rendering kink and inflates the distance calculation.

Fix: track a lastPushed coordinate across legs and skip pushing when the candidate equals it, or slice the first point of every leg's first step after the first leg.

[Nit — low] O(n²) findIndex in pin-drag loop

routes.findIndex((r) => r.vehicleId === route.vehicleId) inside for (const route of routes) is quadratic. Build a vehicleId → index Map before the loop.

[markboenigk]

Review summary

Note on scope: the PR body describes a much larger change (requestDrivingDirections(), sequential-fetch rate-limiting, marker-anchoring fixes, page.tsx mock-fallback removal, .env.example docs) than what's actually in this diff. This is a single commit / single file (Map.tsx, +9/-24) — everything else described already exists on main. Worth updating the description so reviewers aren't scoping against work that isn't here.

Two independent changes are actually in scope:

1. extractRoadPath leg-boundary dedup — looks correct, one edge case

The old code reset its "skip first point" check per-leg (i === 0), so every leg after the first pushed a duplicate point at the waypoint boundary. The fix correctly tracks this globally via legIndex > 0 || stepIndex > 0. Hand-walked a 3-leg example — no dupes, no gaps in the normal case. Note this is cosmetic hygiene, not a functional fix: the duplicate point never affected distance (computed separately from leg.distance.value), the cache key (derived from raw stop coordinates), or bounds-fitting.

One real edge case: if an entire leg has only empty-path steps (e.g. two adjacent stops at identical/near-identical coordinates), skipFirst goes true purely from legIndex > 0 even though nothing has actually been pushed yet, dropping a real point at the next leg's start. Low blast radius (one vertex, not a crash), but skipFirst = path.length > 0 would be a more robust condition than inferring state from legIndex/stepIndex.

2. Effect 2 "ref leak" fix — justification doesn't hold up, and it removes an intentional UX fallback

Traced the race this is meant to fix (Effect 1's sequential Directions fetch mid-flight, a distance update from route 1 reruns Effect 2 while routes 2/3 are still pending): setRoutePolyline already calls setMap(null) on whatever was previously in a vehicle's ref slot before overwriting it, and Effect 1's own cleanup sweeps the entire shared ref on every rerun/unmount. Couldn't find a path through this race where a polyline is actually orphaned/leaked.

Meanwhile, the deleted code's own comment ("Effect 1 may have cleared refs before this runs; show a path immediately") shows the placeholder was intentional. Since Effect 1 fetches routes sequentially (up to a 10s timeout per route), removing Effect 2's fallback means any vehicle not yet reached by Effect 1's loop now shows no polyline at all — not even a straight line — until its turn comes up. For multi-vehicle results this could mean a blank map for several seconds on every load/re-key, not just in the racy edge case this PR targets. That trade-off isn't mentioned in the Risk section.

Could we get either a concrete repro of the leak this fixes, or an approach that keeps immediate visual feedback while routes load sequentially (e.g. Effect 1 drawing a straight-line placeholder for each route before its own await, rather than deleting Effect 2's fallback outright)?

Other

• CI is green across the board.
• No unit tests exist for Map.tsx (pre-existing gap) — extractRoadPath in particular is a pure function that'd be easy to cover for the leg/step boundary cases above.

Net: the extractRoadPath fix looks good to take as-is. Would like to resolve the Effect 2 concern (or see evidence the leak is real) before merging.