Skip to content

Csv parser UI fixes#189

Open
v7ncentng wants to merge 44 commits into
benevolentbandwidth:mainfrom
v7ncentng:csv-parser-uifixes
Open

Csv parser UI fixes#189
v7ncentng wants to merge 44 commits into
benevolentbandwidth:mainfrom
v7ncentng:csv-parser-uifixes

Conversation

@v7ncentng

@v7ncentng v7ncentng commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a two-step CSV/JSON import modal flow to the delivery edit page, accessible from both the Import button on the edit page and the upload-save-point page
  • Users can now upload a .csv or raw .json address file, map file columns to delivery fields in a column mapper (step 1), review and select which rows to import in a scrollable table (step 2), and have confirmed entries autofill the address list
  • Valid session save .json files bypass the modal entirely and restore full vehicle and address state as before

Motivation

  • Route managers had no way to bulk-import addresses from a CSV or raw JSON file without manual entry
  • There was no column mapping step, so mismatched field names silently produced empty or incorrect address cards
  • The upload-save-point page and the edit page triggered different import experiences with no shared logic, causing inconsistent behavior and duplicate code

Changes

CSVUploadOverlay.tsx
edit/page.tsx
upload-save-point/page.tsx

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/mobile run lint
  • npm --prefix app/mobile run typecheck

Manual checks performed:

  • Uploaded a .csv from the edit page Import button → column mapper opened with all columns and 3-row preview populated correctly
  • Uploaded a .csv from upload-save-point → navigated to /edit, column mapper opened automatically with file pre-loaded, no intermediate file-pick screen shown
  • Mapped recipientAddress, clicked Next, deselected two rows, confirmed → only selected rows appeared in the address list
  • Next button remained disabled until recipientAddress was mapped
  • Uploaded a valid session save .json from upload-save-point → modal skipped, full vehicle and address state restored
  • Verified timeBuffer and deliveryQuantity import as numbers matching their AddressCard types
  • Verified failed session import retains savePointFile in sessionStorage so a page refresh retries

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

Risk

  • pendingCSVFile sessionStorage key — new key written by upload-save-point and read on edit page mount. Stale data from a failed navigation will be consumed on the next visit to /edit and open the column mapper unexpectedly. Key is removed immediately after reading so this only affects a single stale session
  • CSVImportModal removed — any code path still importing CSVImportModal from app/edit/components/address/CSVImportModal will fail to compile. Confirm no other files reference it before merging
  • onFileSelect prop removed from CSVUploadOverlay — the old prop signature (onFileSelect: (file: File) => void) is replaced by importAddresses: (addresses: AddressCard[]) => void. Any external callers of CSVUploadOverlay that have not been updated will produce a TypeScript error

Rollout and Recovery

Rollout — Standard frontend deploy. No schema changes, no API changes, no new environment variables. The feature is entirely client-side and scoped to the upload and edit flows.

Recovery — To roll back, revert CSVUploadOverlay.tsx to its previous single-step version and restore the CSVImportModal import in edit/page.tsx. The rest of the edit page (vehicle section, address list, pagination, optimize flow) is unaffected. Upload-save-point can independently revert to its previous version without impacting the edit page.

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.

v7ncentng and others added 30 commits May 14, 2026 12:27
Modified upload_save_point to share the same pipeline as 'import' from the edit page on successful CSV upload.
Ensures ref reads/writes and state updates happen in effect cleanup/execution rather than during render, complying with React's hooks rules.
effects now synchronize with external systems, instead of triggering cascading state updates
@v7ncentng v7ncentng changed the title Csv parser UIfixes Csv parser UI fixes Jun 9, 2026
@markboenigk

Copy link
Copy Markdown
Collaborator

Good consolidation overall — folding the three-step flow into a single component removes real duplication and the naming (StepColumnMapper / StepRowSelector) is cleaner. A few things to address before merging:

High — parse errors are silently swallowed (regression)
The old edit/page.tsx rendered <ErrorOverlay message={parseError} onClose={closeImportModal} /> sourced from useCSVImport. That line is gone. The new CSVUploadOverlay calls useCSVImport internally but never surfaces parseError. A malformed CSV now opens a blank column mapper with no headers and no user-facing feedback. The fix is to expose parseError from the hook and render an ErrorOverlay inside the overlay (or pass an onError prop up to the parent).

High — checklist gaps
typecheck, build, format:check, and test are all unchecked. The prop rename from onFileSelectimportAddresses and the deletion of CSVImportModal could hide TypeScript errors on callsites not in this diff. Please run at minimum typecheck and build before merging.

Medium — wrong MIME type when reconstructing JSON files

const file = new File([content], name, { type: "text/csv" });

Raw JSON arrays (.json extension) reach this path too. The reconstructed File gets type: "text/csv" regardless, which could cause useCSVImport to misparse it. Suggest:

const type = name.endsWith(".json") ? "application/json" : "text/csv";
const file = new File([content], name, { type });

Medium — setState during render

if (headersKey !== prevHeadersKeyRef.current) {
  prevHeadersKeyRef.current = headersKey;
  if (selectedOverride !== null) setSelectedOverride(null);  // setState in render
  ...
}

The comment says this avoids a "setState-in-effect lint error," but calling setState during render is also a React violation. A useEffect(() => { /* reset overrides */ }, [headersKey]) is the cleaner fix.

Low — spinner in handleNext never renders

setIsUploading(true);
openImportModal(selectedFile);
setIsUploading(false);  // batched with the line above — spinner never shows

React 18 batches these into one render. Either drop the setIsUploading calls here (parsing is fast enough to not need a spinner at this step) or make the work genuinely async so the intermediate state is visible.

Low — importedCards path in edit/page.tsx is now dead code
The only writer of sessionStorage.importedCards was CSVImportModal.handleConfirm, which is deleted. The reader in edit/page.tsx can stay for now as a migration safety net, but it's worth a // TODO: remove after one release cycle comment so it doesn't linger.

@markboenigk

Copy link
Copy Markdown
Collaborator

The 2026-06-13 commit addresses all five points from my earlier review — good turnaround. One thing still needs to happen before this can merge:

Please run typecheck and paste the output as a comment:

npm --prefix app/ui run typecheck

The onFileSelectimportAddresses prop rename is a breaking change to CSVUploadOverlay's public interface. The two known callers in this diff were both updated, so it's likely clean — but typecheck is still unchecked on the PR checklist and this is the only way to confirm no other callsite was missed. If it passes, this is ready to merge.

@v7ncentng

Copy link
Copy Markdown
Contributor Author

ui@0.1.0 typecheck
tsc --noEmit

playwright.config.ts:1:39 - error TS2307: Cannot find module '@playwright/test' or its corresponding type declarations.

1 import { defineConfig, devices } from "@playwright/test";
~~~~~~~~~~~~~~~~~~

tests/e2e/optimize-flow.spec.ts:1:41 - error TS2307: Cannot find module '@playwright/test' or its corresponding type declarations.

1 import { test, expect, type Page } from "@playwright/test";
~~~~~~~~~~~~~~~~~~

tests/e2e/optimize-flow.spec.ts:37:60 - error TS7031: Binding element 'page' implicitly has an 'any' type.

37 test("optimize flow routes 2 stops to 1 vehicle", async ({ page }) => {
~~~~

tests/e2e/optimize-flow.spec.ts:40:62 - error TS7006: Parameter 'route' implicitly has an 'any' type.

40 await page.route(/nominatim.openstreetmap.org/search/, (route) =>
~~~~~

Found 4 errors in 2 files.

Errors Files
1 playwright.config.ts:1
3 tests/e2e/optimize-flow.spec.ts:1

@v7ncentng

Copy link
Copy Markdown
Contributor Author

The above error doesn't pop up anymore: typecheck runs as intended.

@v7ncentng

v7ncentng commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Don't merge yet: I need to address a UI issue I just found in the upload-route page. I will probably finish that tonight or tomorrow. Thanks Mark!

@v7ncentng

Copy link
Copy Markdown
Contributor Author

Ready to merge now if no issues are present, thanks!

@markboenigk

Copy link
Copy Markdown
Collaborator

Thanks — all prior points are addressed and typecheck is clean. One new blocker before merge:

High — large CSVs can silently fail to import (regression)
In upload-save-point/page.tsx, sessionStorage.setItem("pendingCSVFile", …) writes the entire raw file (up to the 10 MB limit) into sessionStorage, which has a ~5 MB quota. A valid large CSV throws QuotaExceededError, gets caught by the generic handler, and the import silently fails — the old CSVImportModal handled this case explicitly with a user-facing message. Please wrap the setItem in a try/catch with a "file too large to import" message.

Two optional cleanups (non-blocking):

  • The useCSVImport() call in edit/page.tsx is now vestigial — isImportModalOpen is always false since the overlay owns its own hook instance, so the && !isImportModalOpen term in isDraggingOverPage is dead.
  • The importedCards hydration block in edit/page.tsx is dead code now that CSVImportModal (its only writer) is deleted.

@markboenigk

Copy link
Copy Markdown
Collaborator

The QuotaExceededError blocker is addressed cleanly. One thing still needs fixing before merge, plus two small cleanups:

Medium — parse error dismissal closes the overlay (UX regression)
CSVUploadOverlay.tsx renders:

if (parseError) {
  return <ErrorOverlay message={parseError} onClose={handleClose} />;
}

handleClose calls closeImportModal() and onClose(), so dismissing a parse error (e.g. a malformed CSV) unmounts the overlay and clears pendingCSVFile. The user loses their file and has to start over. The previous flow (edit/page.tsx owned the ErrorOverlay with onClose={closeImportModal}) left the overlay open for retry. Fix:

if (parseError) {
  return <ErrorOverlay message={parseError} onClose={closeImportModal} />;
}

Low — two dead-code paths in edit/page.tsx (non-blocking)

  • const { isImportModalOpen } = useCSVImport() creates a standalone hook instance that nothing drives — isImportModalOpen is always false, making the && !isImportModalOpen term in isDraggingOverPage vacuously true. The import and the term can both be removed.
  • The storedImportedCards block reads a sessionStorage key that CSVImportModal (its only writer) no longer writes. Either delete it or add // TODO: remove after one release cycle so it doesn't linger.

@markboenigk

Copy link
Copy Markdown
Collaborator

Good consolidation overall — deleting CSVImportModal and folding both flows into CSVUploadOverlay is the right call, and the sessionStorage hand-off from upload-save-point is clean. A few things worth addressing before merge:

Bugs

  1. step never resets on re-open. If a user reaches step 2, cancels, then drops a new file, openImportModal is called with fresh CSV data but step is still 2 — they land on the row selector before mapping any columns. Add setStep(1) inside handleClose (or at the start of openImportModal).

  2. selectedFile doesn't track initialFile prop changes. selectedFile is initialized once from initialFile at mount. If the parent updates initialFile while the overlay is open (e.g. a second drag-and-drop while the file picker is visible), the internal state keeps the first file silently. A useEffect syncing initialFile → setSelectedFile would fix it.

  3. Oversized initialFile leaves the user stuck. When initialFile.size > MAX_CSV_BYTES, selectedFile is set to null so the file chip/remove button never renders. The user sees the size error with no way to dismiss it short of closing the entire overlay. Consider rendering the error with its own dismiss button, or keying the chip visibility off initialFile instead of selectedFile.

Minor

  1. // HIGH fix: / // LOW fix: / // MEDIUM fix: comments — these are PR-context labels that lose meaning immediately after merge and violate the project's "no reference to current task/fix" rule in CLAUDE.md. Either drop the prefix or convert them to a plain one-liner explaining the non-obvious constraint (the why, not the priority).

  2. buildAddressCards / MappableField / FIELD_LABELS are now stranded in CSVUploadOverlay. They were the sole shared definitions in CSVImportModal (now deleted). Worth extracting to app/edit/types/csv-import.ts so a future import surface doesn't duplicate them again.

Resets the column-mapper step to 1 whenever the overlay closes, syncs selectedFile/fileSizeError to initialFile prop changes via useEffect, adds a dismiss button to the oversized-file error so users aren't stuck, and removes PR-context HIGH/MEDIUM/LOW fix: comments.
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