HI-FI: Wire Recipient And Delivery Window Data To Results

[KesarSidhu]

Summary

Results expanded stop cards show recipient name, phone, and delivery window from the edit session, mapped through vroomToRoutes into Stop and rendered on each card.

Motivation

Stop cards only showed address, package count, and optimizer arrival time. Recipient contact and the user’s delivery window lived on the edit form but were not passed through to results, so drivers could not see them after optimization.

Changes

• app/ui/src/app/results/types.ts — add optional phoneNumber, deliveryWindowStart, and deliveryWindowEnd on Stop.
• app/ui/src/app/edit/utils/vroomToRoutes.ts — map recipientName, phoneNumber, and delivery window bounds from AddressCard onto each stop (trim empty strings; window only when both start and end are set). Keep capacityUsed from VROOM step.load, not form quantity.
• app/ui/src/app/results/components/EditableStopItem.tsx — render phone and delivery window rows on expanded stop cards.
• app/ui/src/tests/vroomToRoutes.test.ts — test recipient, phone, and window mapping.

Out of scope for this PR (already on main, unchanged here): optimizeMapper.ts, sessionMapper.ts, delivery.ts, useAddresses.ts, useCSVUpload.ts.

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

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

Low — UI-only mapping and display. capacityUsed still comes from the optimizer’s step.load. Phone uses phoneNumber on Stop (same field name as edit → API), not a separate addresseePhone.

Rollout and Recovery

Ship with the results page. Revert this PR if stop card field layout or mapping needs adjustment.

[markboenigk]

Solid plumbing work overall — the round-trip through sessionMapper and the conditional omission of empty fields from the API payload are both done well.

Two things worth a follow-up:

1. New Stop fields are written but never rendered

addresseePhone, deliveryWindowStart, and deliveryWindowEnd are added to results/types.ts and mapped in vroomToRoutes.ts, but EditableStopItem only renders addresseeName — the three new fields are invisible to users. If display is intentional in a follow-up PR that's fine, but worth confirming explicitly. If it was supposed to be in this PR, the rendering is missing.

2. Capacity logic change deserves scrutiny

// before
capacityUsed: step.load?.[0] ?? 0,

// after
const qtyFromForm = address && address.deliveryQuantity > 0 ? address.deliveryQuantity : undefined;
const capacityUsed = qtyFromForm ?? step.load?.[0] ?? 0;

VROOM's step.load is the optimizer's computed load. The form's deliveryQuantity is what the user entered before optimization. Preferring the form value means displayed capacity can diverge from what the optimizer actually computed. Was step.load displaying incorrectly before? The PR description doesn't explain the motivation.

Minor nits:

• useCSVUpload.ts is changed (empty-string defaults for the two new fields) but not mentioned in the PR description — worth keeping the list accurate.
• The same data travels as recipientPhone (AddressCard) → phoneNumber (DeliveryInput) → addresseePhone (Stop). Matches the existing addresseeName pattern but three names for one concept is worth tracking as the model grows.

What's good: defaults added in both useAddresses call sites and useCSVUpload (no AddressCard is created without the required fields), trim() || undefined correctly collapses whitespace-only values, and the both-or-neither window guard is correct.

[kirillakovalenko]

---

Code Review: PR #161 — HI-FI: Wire Recipient And Delivery Window Data To Results

Author: Kesar Singh Sidhu | Files changed: 4 | +41 / -2

---

Overview

A clean, focused PR that pipes recipientName, phoneNumber, and the delivery window from the edit form through vroomToRoutes into the Stop type, then renders them on expanded stop cards. The motivation is solid — drivers had no way to see contact info or delivery windows after optimization.

---

Code Quality

vroomToRoutes.ts — mapping logic

The diff shows that addresseeName and phoneNumber already existed on Stop before this PR (and were already mapped), but the PR correctly adds .trim() to both, which is a good cleanup. The delivery window spread is correct:

...(winStart && winEnd ? { deliveryWindowStart: winStart, deliveryWindowEnd: winEnd } : {})

However, there's a minor inconsistency: inferTimeWindowKind (line 69) still reads the raw address?.deliveryTimeStart / address?.deliveryTimeEnd without trimming, while the new winStart/winEnd variables are trimmed. If a value is " 9:00 AM" (leading space), inferTimeWindowKind would treat it as truthy but the trimmed version would match — so the behavior is consistent in practice, but the raw read in inferTimeWindowKind is slightly inconsistent with the new convention.

EditableStopItem.tsx — rendering

• The phone row renders {stop.phoneNumber ?? "—"} unconditionally, matching the pattern used for addresseeName. Consistent and correct.
• The delivery window row correctly gates on both fields being set before rendering, matching the type contract where both are either present or absent.
• Label concern: The existing label says "Name of addressed to:" (grammatically awkward) — the new "Phone:" and "Delivery window:" labels are fine, but worth flagging the existing label as technical debt.

types.ts — Stop interface

The JSDoc comment added is useful. One note: deliveryWindowStart and deliveryWindowEnd having no paired invariant enforcement at the type level (i.e., you can have one without the other) could be a footgun for future callers. A discriminated union or a nested deliveryWindow?: { start: string; end: string } object would make the "always both or neither" contract explicit and compile-time safe.

// Current (implicit pairing):
deliveryWindowStart?: string;
deliveryWindowEnd?: string;
// More explicit alternative:

deliveryWindow?: { start: string; end: string };

---

Test Coverage

The new test in vroomToRoutes.test.ts is good — it covers the happy path including the trim for recipientName. Missing cases worth adding:

• Only one window bound set (e.g., deliveryTimeStart set, deliveryTimeEnd absent) — verify neither deliveryWindowStart nor deliveryWindowEnd appears on the stop.
• Empty/whitespace-only window strings — verify they're treated as absent (the trim() logic handles this, but a test would lock it in).
• Phone number with surrounding whitespace — the trim() on phoneNumber is untested.

---

Risks & Concerns

Severity	Finding
Low	build and all mobile/backend CI checks are unchecked in the PR checklist — these should pass before merge.
Low	The "always both or never" window invariant is implicit, not type-enforced. Acceptable for now but worth a follow-up.
Low	inferTimeWindowKind uses un-trimmed values while the new mapping uses trimmed ones — cosmetically inconsistent.
Nit	"Name of addressed to:" label (pre-existing) is awkward; could be cleaned up as a drive-by.

---

Summary

The PR is well-scoped, the mapping logic is correct, and the test covers the main happy path. The main actionable items are:

1. Before merge: Confirm the unchecked CI steps pass (especially build).
2. Recommended follow-up: Add edge-case tests for partial window and whitespace-only fields.
3. Optional refactor: Collapse deliveryWindowStart/End into deliveryWindow?: { start: string; end: string } for a stronger type contract.

Verdict: Approve with minor suggestions. The core logic is correct and the change is low-risk.

[markboenigk]

Both concerns from my earlier comment are resolved: fields are rendered in EditableStopItem and VROOM's step.load is kept for capacityUsed. kirillakovalenko's suggestions are also addressed in the latest commit — deliveryWindow uses the nested object shape and inferTimeWindowKind receives trimmed values. CI is all green. Approved.