Skip to content

fix(driver): show invalid route upload errors on upload page#212

Open
hir-al-14 wants to merge 1 commit into
benevolentbandwidth:mainfrom
hir-al-14:fix-upload-route-error
Open

fix(driver): show invalid route upload errors on upload page#212
hir-al-14 wants to merge 1 commit into
benevolentbandwidth:mainfrom
hir-al-14:fix-upload-route-error

Conversation

@hir-al-14

Copy link
Copy Markdown
Contributor

Summary

  • Invalid route uploads now show a clear error on /upload-route instead of sending drivers to the deprecated driver_assist tester upload screen.
  • The old in-page Driver Assist JSON upload fallback has been removed.

Motivation

  • Drivers should see upload failures on the page where they selected the route file so they can retry immediately.
  • Before this change, invalid JSON or invalid route-shaped JSON was only detected after navigating to /driver_assist, which exposed an old unstyled fallback upload UI.

Changes

  • Frontend:

    • Added shared /upload-route validation that parses and transforms route uploads before navigating.
    • Stores invalid upload errors in session storage and renders them in the existing /upload-route error block.
    • Redirects Driver Assist import failures back to /upload-route instead of rendering the deprecated fallback screen.
    • Removed the deprecated Driver Assist direct-upload fallback path, unused file input state, import handler, and related styles.
    • Added regression coverage for rejecting invalid upload-route files before handing them to Driver Assist.
  • 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

  • Main behavior change is limited to the Driver Assist route upload/import flow.
  • Valid route files still import and navigate to /driver_assist.
  • Invalid files now stop on /upload-route with the existing styled error UI.

Rollout and Recovery

  • Roll out normally with the frontend deploy.
  • If anything looks wrong, revert this PR to restore the previous Driver Assist import 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

#198

  • The deprecated in-page tester upload screen in Driver Assist is removed.
  • Uploading an invalid route file via /upload-route shows an error on /upload-route.
  • Valid route files still import and navigate to /driver_assist.
  • Dead code from the removed fallback path is removed.
  • Regression test added for the invalid upload-route error path.

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

Overall: Clean direction — removing the deprecated upload screen from /driver_assist and surfacing errors on /upload-route is the right call. A few things worth fixing before merge.

Bug — sessionStorage.setItem can throw inside a catch block

In driver_assist/page.tsx, the import-error handler does:

} catch (importError) {
  const message = ...;
  clearUploadedRouteFile();
  sessionStorage.setItem(ROUTE_UPLOAD_ERROR_KEY, message); // ← throws in private browsing
  router.replace("/upload-route");
  return;
}

sessionStorage.setItem throws a SecurityError in private-browsing mode (iOS Safari, Firefox strict mode) and a QuotaExceededError if storage is full. Because we're already in a catch, the new exception escapes into React — the redirect never runs and the user is stuck. Wrap the setItem + router.replace in a try/catch, or fall back to a query param if storage is unavailable.

Design — parseRouteUploadText result is discarded; the file is parsed twice

In upload-route/page.tsx:

parseRouteUploadText(text);  // validated and transformed → result thrown away
sessionStorage.setItem("routeFile", JSON.stringify({ name: file.name, content: text }));
router.push("/driver_assist");

Then driver_assist calls loadSessionFromText + transformSessionToDriverRoute on the same string all over again. The two functions are identical in both places, so no behavioral difference today — but this is fragile: any future divergence (extra validation step, schema migration) will silently create inconsistent behavior between pages. Consider storing the already-transformed result in sessionStorage (or at least a comment explaining why re-parsing is intentional).

Nit — ROUTE_UPLOAD_ERROR_KEY belongs in a shared location

driver_assist importing from @/app/upload-route/routeUploadValidation inverts the expected sibling-route boundary. If routeUploadValidation ever gains a transitive import from driver_assist, you have a circular dependency. Moving the constant (and arguably the key itself) to lib/driver-route/ or a shared constants file removes that risk and makes the coupling explicit.

Nit — queueMicrotask wrappers are unnecessary

React 18 batches all setState calls inside useEffect automatically. The queueMicrotask(() => { setRoute(...); setHasCheckedRoute(true); }) pattern in both the success and saved-route branches adds a render-cycle delay and makes the timing harder to reason about without any benefit. Calling the setters directly works identically — same for the queueMicrotask(() => setError(...)) in upload-route/page.tsx.

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