HI-FI: Mobile Implementation#178
Conversation
3820d9e to
830c9a2
Compare
kirillakovalenko
left a comment
There was a problem hiding this comment.
PR #178 — HI-FI: Mobile Implementation
Author: KesarSidhu | +2254 / -306 lines | 28 files
Overview
This PR adds a responsive mobile layout for /results (full-screen map + bottom sheet), restores hi-fi design (teardrop pins, route-colored polylines, stop timeline, package badges), extracts RouteCard and RouteCardMenu from Sidebar, adds Export/Duplicate/Delete route actions with a portal-based menu, and switches from a mock-data fallback to sessionStorage-only route loading. The motivation and scope are well-described. Architecture is sound.
Bugs
Wrong icon for "Duplicate Route" and "Delete Route"
RouteCardMenu.tsx:1952, 1965 — Both use DownloadIcon. Delete and Duplicate need their own icons (trash, copy); as written, all three actions in the ⋮ menu look identical.
Desktop Save button always visible
page.tsx:2738-2742 — The Save button is always rendered in the desktop header (disabled when idle), which is a change from the previous "only show Cancel/Save during a pending move" pattern. Whether intentional, it makes the toolbar look active even when nothing is pending. Worth confirming with design.
isSidebarOpen is now dead state
page.tsx:2563 — const isSidebarOpen = true; replaces a useState. The toggle button was removed, which is fine, but this constant is only used in one className ternary. The isSidebarOpen ? "w-[28rem]" : "w-0" branch can never take w-0 — either remove the ternary or remove the variable.
queueMicrotask inside useEffect is unnecessary
page.tsx:2549 — useEffect already defers to after mount. Wrapping the sessionStorage read in queueMicrotask adds no value and makes the code harder to follow. Plain setRoutes(parsed) is fine.
Code Quality Issues
useIsClient duplicated across two files
ExportRoutesModal.tsx:702-708 and ExportEditWarningModal.tsx:535-541 — Identical hook defined twice. Extract to app/results/hooks/useIsClient.ts.
Hard-coded hex in RouteCard
RouteCard.tsx:1622 — "border-[#6CCBBE]" is inconsistent with the rest of the codebase that uses CSS variables (var(--edit-teal-300) etc.).
Menu height hard-coded as magic number
RouteCardMenu.tsx:1881 — const menuHeight = 148; will silently break placement="up" positioning if the menu content ever changes.
SidebarResultsButton reads sessionStorage on every render
SidebarResultsButton.tsx:103 — readHasOptimizeResults() is called synchronously inline on each render. Wrap in useState + useEffect to read once on mount and avoid re-reading on every parent re-render.
Redundant condition in vroomToRoutes
vroomToRoutes.ts:227-230:
address?.deliveryQuantity && address.deliveryQuantity > 0
The && before > 0 is redundant (falsy check already excludes 0). Cleaner as:
(address?.deliveryQuantity ?? 0) > 0
? address.deliveryQuantity
: (step.load?.[0] ?? 0)
Dead tokens in formStyles.mobile.ts
formStyles.mobile.ts:2455-2490 — ~15 constants (RESULTS_ROUTE_CARD_ROOT, RESULTS_ROUTE_CARD_HEADER_BTN, etc.) are defined but never referenced — RouteCard.tsx uses inline Tailwind instead. These should be removed or actually used.
handleEditToggle not memoized
ResultsBottomSheet.tsx:1416 — Defined as a plain function inside the component body; wrap in useCallback for consistency with the rest of the codebase.
downloadRouteJson staggered timeouts not cleaned up
downloadRouteJson.ts:2900 — window.setTimeout IDs are discarded. Fire-and-forget downloads are acceptable here, but if the component ever gains a cleanup path this will be a latent bug.
Accessibility
First expand button in RouteCard is missing aria-label
RouteCard.tsx:1655 — Has aria-expanded but no aria-label; a screen reader will have no name for the button. The chevron button right next to it has a label — the first expand button needs one too.
Validation Gaps
npm run test and npm run build are unchecked in the PR — these should pass before merge, especially since new utilities and hooks are added.
Mobile lint/typecheck unchecked — while no mobile (native) changes are in this PR, confirming is still worth it.
What's Done Well
recipientSummary, vroomToRoutes, and addressCardToDeliveryInput tests added and thorough.
readHasOptimizeResults has a safe try/catch and SSR guard — good defensive coding.
SVG data URL for map pins has an inline comment noting it's never user-supplied (no XSS risk).
Proper hover listener cleanup in AdvancedMarkers with hoverCleanupRef.
Portal-based RouteCardMenu so the menu is never clipped in the bottom sheet — correct approach.
Focus trap and Escape-to-close on both modals.
aria-hidden throughout on decorative SVGs; aria-label on interactive map markers.
Summary
The mobile layout and hi-fi work are solid, and the sessionStorage loading refactor is cleaner. The main actionable items before merge are: fix the wrong icons for Duplicate/Delete (functional issue), run and pass tests/build, and clean up useIsClient duplication, dead token exports, and the dead isSidebarOpen variable. The magic menu height and hard-coded hex are minor polish items.
Align the results header with the mock by using a compact app label plus Save/Export actions while preserving the existing sidebar toggle and pending-edit cancel behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Refine the sidebar title section, edit action, and footer copy styling to better match the hifi mock while keeping the route list behavior unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Use route palette colors for map pins and switch marker visuals from plain dots to pin-style markers so map markers align with route styling. Co-authored-by: Cursor <cursoragent@cursor.com>
Match the edit-mode layout with a wider results panel, save-edits CTA styling, and centered map status banner while keeping existing route behavior intact. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Add cyan timeline markers with dotted connectors in expanded routes. Remove duplicate sequence badge from EditableStopItem. Co-authored-by: Cursor <cursoragent@cursor.com>
Extract recipientSummary for tests, hide locked contact row when empty, and use middle-dot formatting on hi-fi address cards. Co-authored-by: Cursor <cursoragent@cursor.com>
Map recipient_phone CSV alias, prefer form delivery quantity for capacityUsed, and add unit tests for recipient fields and capacity fallback. Co-authored-by: Cursor <cursoragent@cursor.com>
Add optional delivery window fields on Stop, format recipient and delivery lines on EditableStopItem, and map form delivery times into routes. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Match hi-fi expanded stop cards with icon rows for recipient and delivery, route-tinted package pill, and rounded card layout. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Add note doc icon row, hi-fi read-only note panel, and teal save control in edit mode for expanded stop cards. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Wire hi-fi results Export button to route JSON downloads, with a warning when edit mode or pending pin moves are active. Rebased on step3-edit-look-3-main-v7; keep existing routeColors palette. Co-authored-by: Cursor <cursoragent@cursor.com>
Rebased on step4-export-popup: per-route export/duplicate/delete menu, marker hover tooltips, and existing routeColors/Map directions unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Rebased on step5: mobile navbar, bottom sheet, nav rail, and RouteCard with desktop sidebar unchanged in behavior; keep step5 Map and directions. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
fdc1c4b to
6efe39d
Compare
Summary
/results(map + bottom sheet route list) while keeping the existing desktop sidebar experience at thelgbreakpoint.sessionStorageafter the optimize flow.Motivation
step6-results-mobileso desktop behavior fromstep5-route-menu-marker-hoverstays stable while mobile UX ships.Changes
Frontend (
app/ui)MobileResultsNavbar,ResultsBottomSheet,formStyles.mobile.ts, andlg:hiddenlayout inpage.tsx.RouteCardextracted fromSidebar; sheet variant viaSidebarvariant="sheet"; route menu uses a portal so Export/Duplicate/Delete are not clipped in the bottom sheet.EditableStopItempackage pill tinted by route color; teardrop map pins and per-route polyline colors inMap.tsx.optimizeResultsfromsessionStorageafter mount; deletedmock_route.jsonandmockRouteLoader.ts.RouteCardMenu,page.tsxsession load, unused vars).Backend / mobile app / infra
Validation
Frontend
npm --prefix app/ui run lintnpm --prefix app/ui run format:checknpm --prefix app/ui run typechecknpm --prefix app/ui run testnpm --prefix app/ui run buildnpm --prefix app/mobile run lintnpm --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'docker compose -f deploy/compose/docker-compose.arm64.yml --env-file deploy/env/http-server.arm64.env configManual checks
lg: map fills screen, bottom sheet expands/collapses, Edit and Export work, route ⋮ menu is fully visible.lg: desktop sidebar + nav rail unchanged; Export in header works./edit, land on/results: routes render fromsessionStorage(no mock data).NEXT_PUBLIC_GOOGLE_MAPS_MAP_IDset in.env.local.Risk
/resultsis empty on direct load without prior optimize (mock fallback removed); users must complete optimize or return to/edit.sessionStorageis consumed once after load; refresh clears routes unless optimize is run again.useLayoutEffect+ portal; unusual scroll containers could affect placement (mitigated bymenuOpensUpin the bottom sheet).Markericons still render but behavior may differ slightly.Rollout and Recovery
app/ui; no migrations or feature flags./resultsreturns to prior sidebar-only layout.ResultsBottomSheet/ mobile block inpage.tsxwhile keeping desktop path.High-Signal PR Checklist