Repository navigation
Conversation
markboenigk
left a comment
There was a problem hiding this comment.
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.
b44e7b0 to
4e203c6
Compare
markboenigk
left a comment
There was a problem hiding this comment.
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.
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 toreadSavedRoute()when the upload failed to parse and still rendered the dashboard. In the newloadInitialRoute(), onceuploadedRouteis present but fails to parse, the function returns early withsource: 'invalid-upload'and never callsreadSavedRoute(); theuseEffectthen unconditionallyrouter.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'sloadInitialRoute) — 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).
e864f12 to
28d8379
Compare
|
Thanks for this — the weather/traffic forecast layering is solid. A few things before it can go in: Blocking
Follow-up (not blocking)
|
28d8379 to
51bd11a
Compare
markboenigk
left a comment
There was a problem hiding this comment.
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:
- Stale weather impact reused after rerun (
optimization_job_runtime.cpp,WorkerLoop):impactis computed once before the weather-triggered VROOM rerun. Ifimpact.should_reoptimizefires and reassignsfinal_result, the subsequentPrepareTrafficPostprocessing(traffic_options_, parsed_request->input, impact, *final_result.output, *forecast)still passes the pre-rerunimpactagainst the post-rerun route. Recomputeimpactagainstfinal_result.outputafter the rerun before using it downstream. - Unbounded traffic thread spawn across workers (
forecast_optimizer.cpp,ReadRouteTraffic): each call spawns up tokMaxConcurrentTrafficRequests(4)jthreads with no cap shared across concurrent job-runtime workers or the sync endpoint. Withworker_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. - Traffic delay weighting numerator/denominator mismatch (
BuildWeightedTrafficDelays): the delay total is summed over every leg including the final job→route-end leg, butReadJobTravelSecondsexcludes that same leg from the travel-time denominator (only counts legs ending in ajobstep). Over-attributes delay to job stops that never experienced it whenever the end leg has meaningful traffic delay. - Fallback delay distribution doesn't conserve the total (
BuildEvenTrafficDelays):ceil(total / job_count)per stop means the sum of injected delays can exceedtotal_delay_seconds(e.g. 105s/20 jobs → 120s injected). Use floor + largest-remainder like the primary weighted path does. - CSV
sequenceoff-by-one (app/ui/src/app/upload-route/routeUploadValidation.ts:68):sequence > 0 ? sequence - 1 : indextreats a validsequence = 0as absent, colliding with a row wheresequence = 1legitimately maps to index 0 — scrambles stop order.#174already 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.
Summary
Motivation
RouteJSON shape, but Driver Assist only accepted session-save or raw session data JSON.Changes
Frontend:
sequence-> route orderaddresseeName-> customer namephoneNumber-> phone numbercapacityUsed-> package countnote-> noteslat/lng-> navigation coordinates/upload-routeand/driver_assistto use the same Driver Assist route parser.Backend:
Validation
Frontend
npm.cmd --prefix app/ui run lintnpm.cmd --prefix app/ui run format:checknpm.cmd --prefix app/ui run typechecknpm.cmd --prefix app/ui run testnpm.cmd --prefix app/ui run buildnpm.cmd --prefix app/mobile run lintnpm.cmd --prefix app/mobile run typecheckBackend
cmake --preset dev.github/scripts/check-backend-static.sh build/devcmake --build --preset dev --parallelctest --preset dev --output-on-failure --no-tests=error -LE 'e2e|docker'Risk
Rollout and Recovery
High-Signal PR Checklist
Related Issues
#199
lat/lng.