Skip to content

Fixing Path Between Destinations#202

Open
KesarSidhu wants to merge 6 commits into
benevolentbandwidth:mainfrom
KesarSidhu:fixing-road-path
Open

Fixing Path Between Destinations#202
KesarSidhu wants to merge 6 commits into
benevolentbandwidth:mainfrom
KesarSidhu:fixing-road-path

Conversation

@KesarSidhu

@KesarSidhu KesarSidhu commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Collaborator

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.

Kesar Sidhu and others added 4 commits June 22, 2026 11:16
Remove duplicate translate on Advanced Marker pin content (default anchor is
already bottom-center) and always set scaledSize plus anchor on legacy Marker
icons so stop pins stay pinned to their geographic coordinates when zooming.

Co-authored-by: Cursor <cursoragent@cursor.com>
Load routes only from sessionStorage after optimize; show an error overlay
when no stored data or corrupt JSON instead of silently rendering mock routes.

Co-authored-by: Cursor <cursoragent@cursor.com>
Use callback-based directions requests with status checks, extract road paths
from overview or leg steps, fetch routes sequentially to avoid rate limits,
stop overwriting cached road paths with straight lines, and document Directions
API requirement in .env.example.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

@markboenigk markboenigk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)));
  }
}

Kesar Sidhu and others added 2 commits June 26, 2026 09:41
…smatch

Restore straight-line polyline fallback on directions cache miss after pin
commit, recreate polylines when the directions effect clears refs first,
dedupe step-path vertices in extractRoadPath, and defer missing-data errors
to a client-only useEffect to prevent SSR hydration mismatch.

Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the client-only error useEffect with useSyncExternalStore so
validation runs after hydration without triggering react-hooks/set-state-in-effect.

Co-authored-by: Cursor <cursoragent@cursor.com>
@markboenigk

Copy link
Copy Markdown
Collaborator

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Path Between Destinations Does Not Follow Roads

2 participants