Skip to content

Fix driver route export import - #213

Open
hir-al-14 wants to merge 42 commits into
benevolentbandwidth:mainfrom
hir-al-14:fix-driver-route-export-import
Open

hir-al-14 wants to merge 42 commits into
benevolentbandwidth:mainfrom
hir-al-14:fix-driver-route-export-import

Conversation

@hir-al-14

Copy link
Copy Markdown
Contributor

Summary

  • Driver Assist can now import the exact JSON file exported from a single route on the Results page.
  • Existing Driver Assist upload formats still work.

Motivation

  • A route exported from Results should be usable by the driver without manual editing.
  • Before this change, Results exported a Route JSON shape, but Driver Assist only accepted session-save or raw session data JSON.

Changes

  • Frontend:

    • Added support for Results page route export JSON in the Driver Assist importer.
    • Maps exported route stops into Driver Assist stops:
      • sequence -> route order
      • addresseeName -> customer name
      • phoneNumber -> phone number
      • capacityUsed -> package count
      • note -> notes
      • lat / lng -> navigation coordinates
    • Updated /upload-route and /driver_assist to use the same Driver Assist route parser.
    • Kept existing session-save envelope and raw session data imports working.
    • Added regression coverage for importing a Results route export.
  • Backend:

    • No backend changes.

Validation

Frontend

  • npm.cmd --prefix app/ui run lint
  • npm.cmd --prefix app/ui run format:check
  • npm.cmd --prefix app/ui run typecheck
  • npm.cmd --prefix app/ui run test
  • npm.cmd --prefix app/ui run build
  • npm.cmd --prefix app/mobile run lint
  • npm.cmd --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'

Risk

  • Low risk.
  • Change is limited to Driver Assist route import parsing.
  • Existing accepted upload formats are preserved.
  • Results export format is not changed.

Rollout and Recovery

  • Roll out normally with the frontend deploy.
  • If anything looks wrong, revert this PR. Existing session-save and raw session imports should return to the old behavior.

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.

Related Issues

#199

  • Exporting a route from Results and uploading that file to Driver Assist populates the route.
  • Stops keep the correct order, address, recipient name, phone number, package count, notes, and coordinates.
  • Navigation uses exported lat / lng.
  • Existing session-save and raw session data uploads still work.
  • Invalid/unrecognized JSON still shows a validation error.
  • Regression test added for importing a Results route export.

@hir-al-14 hir-al-14 mentioned this pull request Jun 27, 2026
11 of 18 tasks

@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.

Code Review

This PR adds loadDriverRouteFromText to accept both Results-page export JSON and the existing session save format, removes the inline upload UI from /driver_assist, and routes import errors back via sessionStorage. Clean frontend-only change — here are a few things worth addressing before merge.


Bug: capacityUsed: 0 silently rejects valid Results exports

app/ui/src/lib/driver-route/importSession.ts line 47:

capacityUsed: z.number().positive().optional(),

z.number().positive() excludes zero. If the Results page ever exports a stop with capacityUsed: 0 (e.g. a mandatory waypoint or pickup with nothing to drop off), resultsRouteSchema.safeParse() fails, the code falls through to session parsing which also fails, and the driver sees "This file is not a recognized route or session JSON file" — no indication of the actual problem.

Fix: change .positive() → .nonnegative(). Also revisit whether the ?? 1 default on line 183 should become ?? 0 (0-capacity stop vs. unspecified-capacity stop are different things).


Architecture: double validation makes the driver_assist error path dead code

upload-route/page.tsx now calls parseRouteUploadText(text) (result discarded) before storing to sessionStorage. Then driver_assist/page.tsx calls loadDriverRouteFromText again on the same bytes. Since both call the same function with the same input, anything that passes upload-route will pass driver_assist — the catch / sessionStorage.setItem(ROUTE_UPLOAD_ERROR_KEY, ...) / router.replace block in driver_assist is unreachable through the normal UI flow.

Worth picking one owner: pre-validate at upload and simplify/remove the driver_assist catch, or drop the upload-time pre-validation and let driver_assist remain the sole error handler. Right now both live on and the dead path will silently rot.


Architecture: sessionStorage for cross-page error passback is fragile

Writing an error key in driver_assist and reading it back in upload-route's mount effect breaks silently if sessionStorage is unavailable (strict private-browsing settings, storage quota hit) or cleared mid-navigation. The Next.js convention for this pattern is a query param:

router.replace(`/upload-route?error=${encodeURIComponent(message)}`);

Survives storage resets, shows up in the address bar, and eliminates the ROUTE_UPLOAD_ERROR_KEY constant being imported from an upload-route internal module into driver_assist.


Cleanup: queueMicrotask wrapping setState in useEffect is unnecessary

React 18 automatically batches all synchronous state updates within an effect. The queueMicrotask calls on lines 74 and 100 of driver_assist/page.tsx add an extra microtask turn with no batching benefit — they just delay the loading → route transition by one tick and make the timing harder to follow. Calling the three setters directly is equivalent and clearer.


Cleanup: parseRouteUploadText is a pure pass-through

routeUploadValidation.ts exports one constant and one function that is literally return loadDriverRouteFromText(text). The module name implies validation responsibility that lives entirely in importSession.ts. Callers could import loadDriverRouteFromText directly; ROUTE_UPLOAD_ERROR_KEY could move to a shared constants file to remove the driver_assist → upload-route import dependency.

@hir-al-14
hir-al-14 force-pushed the fix-driver-route-export-import branch from b44e7b0 to 4e203c6 Compare August 8, 2026 01:28

@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.

Code review — 1 correctness issue (same root cause as PR #212)

Reviewed the full diff on this branch (6 commits on top of main), covering driver_assist/page.tsx, storage.ts, and lib/driver-route/importSession.ts.

  1. app/ui/src/app/driver_assist/page.tsx:34 — An invalid uploaded route now always redirects to /upload-route, even when a valid saved route already exists in localStorage. The old code fell through to readSavedRoute() when the upload failed to parse and still rendered the dashboard. In the new loadInitialRoute(), once uploadedRoute is present but fails to parse, the function returns early with source: 'invalid-upload' and never calls readSavedRoute(); the useEffect then unconditionally router.replace()s to /upload-route, bouncing the driver away from their active route instead of showing it. No test covers the case of a valid saved route coexisting with an invalid uploaded file. This is the same regression already flagged on PR #212 (driver_assist/page.tsx's loadInitialRoute) — worth fixing at the shared root cause rather than per-PR.

Everything else in the diff checked out: SSR guards on storage reads are correct, capacityUsed ?? 1 correctly preserves zero via nullish coalescing (covered by a new test — this looks like it fixes the zero-capacity issue flagged on PR #174), the Suspense wrapper for useSearchParams() is properly done, the router.push target fix (/driver-view → /driver_assist) is legitimate and no other code targets the old page, and removing loadSessionFromFile/transformSessionToDriverRoute from this file doesn't break other call sites (they remain in use via app/edit/page.tsx's separate import module).

@hir-al-14
hir-al-14 force-pushed the fix-driver-route-export-import branch from e864f12 to 28d8379 Compare August 28, 2026 02:34
@markboenigk

Copy link
Copy Markdown
Collaborator

Thanks for this — the weather/traffic forecast layering is solid. A few things before it can go in:

Blocking

  • Rebase needed — merge conflict with main. perf(api): reduce solve pipeline allocations #235 refactored app/api/src/endpoints/deliveries_optimize_endpoint.cpp onto the new SyncSolveContext model (CompleteAndRespond / BuildVroomInputText, 16-byte std::function captures). The traffic-forecast flow here (FinishWithTraffic, the traffic_options capture, the re-run factory) still uses the old respond_with_completion / optimize_request_ptr / BuildVroomInput pattern and needs to be reworked onto the new architecture — the conflict won't resolve mechanically. Every other file auto-merges.
  • A failed or timed-out traffic re-solve returns an error to the client even though the weather/base route was already fully optimized (deliveries_optimize_endpoint.cpp sync path and optimization_job_runtime.cpp job path). Traffic forecasting is optional and env-gated — it must not be able to fail an otherwise-successful request. Fall back to the weather/base result when the traffic re-run produces no output.
  • forecast_optimizer.cpp ReadTrafficDelay: the top-level Google API status is never checked. REQUEST_DENIED / OVER_QUERY_LIMIT / INVALID_REQUEST come back with no rows, so a misconfigured GOOGLE_MAPS_API_KEY silently produces a zero-delay forecast with no log signal. Check status and log on error.
  • forecast_optimizer.cpp ReadLegDeparture: the departure time can land in the past in the async job path (job solved well after route_start_time). Distance Matrix rejects a past departure_time and omits duration_in_traffic, zeroing traffic for the whole route even when the feature is correctly configured. Clamp to now + a small buffer.

Follow-up (not blocking)

  • ReadRouteTraffic issues one Distance Matrix request per leg, synchronously in batches of 4 on a coordinator worker — a 40-stop route can block a worker ~50s and multiplies API cost/quota. Distance Matrix accepts batched origin/destination pairs (up to 25x25) in a single request.
  • ReadRouteTraffic spawns jthread workers then immediately calls workers.clear(), relying implicitly on ~jthread join semantics. An explicit join loop (or letting workers fall out of scope) would keep the invariant safe against a later refactor to std::thread / detach / move.
  • app/ui/src/app/driver_assist/components/StopCard.tsx:114: the Call action changed from <button> to <a href={\tel:${phoneNumber}`}>butstyles.actionButtonhas notextDecoration: 'none'`, so it renders underlined next to the adjacent Navigate/Report buttons.

@hir-al-14
hir-al-14 force-pushed the fix-driver-route-export-import branch from 28d8379 to 51bd11a Compare September 7, 2026 22:22

@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.

This is the furthest-along line of the driver-assist/traffic-forecast work (#181 → #212 → #213) and already picked up #212's fix for the invalid-upload/saved-route bug (refactored into initialRoute.ts), plus this PR's own hardening of ReadLegDeparture (minimum-departure clamp, Google Distance Matrix status error surfacing) — good improvements. Requesting changes on what's still open, all in app/api/src/forecast_optimizer.cpp / optimization_job_runtime.cpp unless noted:

  1. Stale weather impact reused after rerun (optimization_job_runtime.cpp, WorkerLoop): impact is computed once before the weather-triggered VROOM rerun. If impact.should_reoptimize fires and reassigns final_result, the subsequent PrepareTrafficPostprocessing(traffic_options_, parsed_request->input, impact, *final_result.output, *forecast) still passes the pre-rerun impact against the post-rerun route. Recompute impact against final_result.output after the rerun before using it downstream.
  2. Unbounded traffic thread spawn across workers (forecast_optimizer.cpp, ReadRouteTraffic): each call spawns up to kMaxConcurrentTrafficRequests (4) jthreads with no cap shared across concurrent job-runtime workers or the sync endpoint. With worker_count >= 2, concurrent traffic-eligible jobs can spawn up to N×4 simultaneous Google Maps requests. Needs a process-wide bound (e.g. a counting semaphore around the actual HTTP fetch) rather than a per-call pool.
  3. Traffic delay weighting numerator/denominator mismatch (BuildWeightedTrafficDelays): the delay total is summed over every leg including the final job→route-end leg, but ReadJobTravelSeconds excludes that same leg from the travel-time denominator (only counts legs ending in a job step). Over-attributes delay to job stops that never experienced it whenever the end leg has meaningful traffic delay.
  4. Fallback delay distribution doesn't conserve the total (BuildEvenTrafficDelays): ceil(total / job_count) per stop means the sum of injected delays can exceed total_delay_seconds (e.g. 105s/20 jobs → 120s injected). Use floor + largest-remainder like the primary weighted path does.
  5. CSV sequence off-by-one (app/ui/src/app/upload-route/routeUploadValidation.ts:68): sequence > 0 ? sequence - 1 : index treats a valid sequence = 0 as absent, colliding with a row where sequence = 1 legitimately maps to index 0 — scrambles stop order. #174 already has the correct fix (sequence >= 0) in its version of this file; please port that check over.

Once these land, I think this supersedes #181 and #212 outright — recommend closing those in favor of this one rather than merging all three.

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.

2 participants