HI-FI: Finish Expanded Stop Note Section Styling

[KesarSidhu]

Summary

Expanded stop cards finish with hi-fi note styling in read-only and edit modes: icon-led note row, updated typography, and a teal save control.

Motivation

After v6 restyled the header and recipient/delivery rows, the notes area still used the older compact layout and amber save button. Drivers editing stop notes should see the same visual system as the rest of the expanded card.

Changes

• app/ui/src/app/results/components/EditableStopItem.tsx — NoteDocIcon and rounded note panel in read-only mode (Note: label, zinc background); edit mode uses the same icon row, larger textarea with teal focus ring, and rounded-full #7BCFC2 Save button; mt-3 spacing aligned with detail rows above.

Validation

Frontend

• npm --prefix app/ui run lint
• npm --prefix app/ui run typecheck
• npm --prefix app/ui run format:check
• npm --prefix app/ui run build
• Manual: expand a route on /results in view and edit mode — note block and Save match hi-fi
• 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 — styling-only changes to the note section; note save behavior unchanged.

Rollout and Recovery

Merge after step3-edit-look-3-main-v6 in the step3 stack. Revert if note layout or Save styling needs adjustment.

[kirillakovalenko]

Code Review — PR #166: HI-FI: Finish Expanded Stop Note Section Styling

Author: Kesar Singh Sidhu | +721 / −264 | 15 files

---

Overview

The PR title ("Finish Note Section Styling") significantly understates what's actually here. In addition to the note styling in EditableStopItem, the diff includes: a full results-page sidebar redesign, custom SVG map pin implementation, navigation awareness in SidebarResultsButton, a new delivery-window data flow (deliveryWindowStart/End), a capacityUsed fix, CSV alias addition, two new utility modules, and a branding footer. This is a medium-sized feature roll-up rather than a styling touch-up.

---

Critical Issues

1. "Save" pin-move button is always visible

page.tsx:1093-1099 — The "Save" header button was moved outside the pendingPinMove != null guard that previously wrapped it. It now renders on every page load with no handler guard. Clicking it when there's no pending move is a silent no-op, but it presents misleading UI — a Save button that appears to do nothing.

2. "Export" button is a stub with no handler

page.tsx:1100-1106 — The Export button has no onClick. Users will click it, nothing will happen, and there's no disabled state or tooltip to explain why. Should be hidden until the feature is implemented, or at minimum disabled with aria-disabled and a tooltip.

3. Sidebar toggle permanently removed with dead code left behind

isSidebarOpen is now hardcoded to true, but the width ternary isSidebarOpen ? "w-[28rem]" : "w-0" still references it. The toggle button and its setIsSidebarOpen call are gone. If permanently removing collapsibility is intentional, clean up the ternary and remove the state variable entirely.

---

Correctness Issues

4. aria-disabled on a <span> is non-semantic

SidebarResultsButton.tsx:138 — aria-disabled="true" on a <span> has no effect; spans are not interactive elements so screen readers won't announce the disabled state. Use a <button disabled> or <button aria-disabled="true"> with pointer-events: none instead.

5. formatTimeWindowLine has an implicit fallthrough

EditableStopItem.tsx:263-265:

if (tw.kind === "by") return `By ${label}`;
if (tw.kind === "at") return label;
return `From ${label}`; // catches anything that isn't "by" or "at"

Any unexpected kind value silently renders as "From …". Add an explicit if (tw.kind === "from") branch and a defensive fallback (e.g., return label or log an error) to avoid silently swallowing unknown values.

6. capacityUsed condition is redundant

vroomToRoutes.ts:214-216:

address?.deliveryQuantity && address.deliveryQuantity > 0

If deliveryQuantity is truthy (non-zero number), the > 0 check is redundant. Harmless but noisy — simplify to address?.deliveryQuantity || step.load?.[0] ?? 0.

---

Design / Architecture Concerns

7. formatContactLine duplicates recipientSummary logic

EditableStopItem.tsx:274-281 — formatContactLine in the results component duplicates the exact same name · phone join logic just extracted into recipientSummary.ts. The Stop type fields differ from AddressCard, but extracting a shared formatter that takes { name?: string; phone?: string } would eliminate the duplication.

8. Module-level mutable markerScaledSize singleton

Map.tsx:512 — let markerScaledSize at module scope is a lazy singleton that captures a google.maps.Size. This is fine in production, but during HMR the module won't re-evaluate and the cached size object could hold a stale reference if the Google Maps SDK reloads. Low risk for now, but worth a comment noting the intent.

9. Stop sequence numbers hardcoded to bg-cyan-500

Sidebar.tsx:941 — The sequence badge uses a hardcoded teal regardless of route accent color. This means all routes look identical at the stop level. If the hi-fi design calls for uniform teal, that's fine — but it's surprising given that the route header, polylines, and map pins all use accent. Confirm this is intentional.

10. Inline SVG icons defined locally instead of shared

PersonIcon, ClockIcon, NoteDocIcon, PackageIcon in EditableStopItem.tsx and the route/truck/chevron SVGs inline in Sidebar.tsx are all new one-off definitions. If the project has or plans a shared icon library, these belong there. At minimum, icons used in both components (like the route icon) should not be duplicated.

---

Minor / Style

• b2-logo.png committed to public/ — Binary assets in the repo are fine, but confirm this is intentional and not a placeholder that should be loaded from a CDN.
• "use client" added to SidebarResultsButton — Correct, since it uses usePathname. Good catch.
• fill="currentColor" fix in SidebarResultsButton — Correct improvement; the previous fill="var(--edit-text-primary)" would have broken in contexts where the CSS variable isn't in scope.
• recipient_phone CSV alias — Good addition; consistent with the existing alias pattern.
• hasOptimizeResults.ts SSR guard (typeof window === "undefined") — Correct defensive check for Next.js server rendering.

---

Test Coverage

Area | Status -- | -- recipientSummary / hasRecipientContact | ✅ Well-covered capacityUsed fallback logic | ✅ Covered addressCardToDeliveryInput recipient forwarding | ✅ Covered formatTime12h / formatDeliveryWindow | ❌ Not tested SidebarResultsButton routing state logic | ❌ Not tested hasOptimizeResults | ❌ Not tested

---

PR Checklist Gaps

The submission has 8 unchecked validation items, notably format:check, build, manual testing on /results, and all mobile/backend checks. Given that this PR introduces navigation routing logic, a layout overhaul, and data model additions, the "Edit" → "Save edits" flow and custom map pin rendering at minimum should be manually verified before merge.

---

Summary

The code quality is generally solid — the utility extractions are clean, SVG icons are well-crafted, the deliveryWindowStart/End data flow is properly typed end-to-end, and new tests are meaningful. The two blocking concerns are the always-visible Save button and the stub Export button, both of which will confuse users immediately. The aria-disabled issue and formatTimeWindowLine fallthrough are smaller but worth fixing before merge.

Code Review — PR #166: HI-FI: Finish Expanded Stop Note Section Styling Author: Kesar Singh Sidhu | +721 / −264 | 15 files

Overview
The PR title ("Finish Note Section Styling") significantly understates what's actually here. In addition to the note styling in EditableStopItem, the diff includes: a full results-page sidebar redesign, custom SVG map pin implementation, navigation awareness in SidebarResultsButton, a new delivery-window data flow (deliveryWindowStart/End), a capacityUsed fix, CSV alias addition, two new utility modules, and a branding footer. This is a medium-sized feature roll-up rather than a styling touch-up.

Critical Issues

1. "Save" pin-move button is always visible

page.tsx:1093-1099 — The "Save" header button was moved outside the pendingPinMove != null guard that previously wrapped it. It now renders on every page load with no handler guard. Clicking it when there's no pending move is a silent no-op, but it presents misleading UI — a Save button that appears to do nothing.

2. "Export" button is a stub with no handler

page.tsx:1100-1106 — The Export button has no onClick. Users will click it, nothing will happen, and there's no disabled state or tooltip to explain why. Should be hidden until the feature is implemented, or at minimum disabled with aria-disabled and a tooltip.

3. Sidebar toggle permanently removed with dead code left behind

isSidebarOpen is now hardcoded to true, but the width ternary isSidebarOpen ? "w-[28rem]" : "w-0" still references it. The toggle button and its setIsSidebarOpen call are gone. If permanently removing collapsibility is intentional, clean up the ternary and remove the state variable entirely.

Correctness Issues
4. aria-disabled on a is non-semantic

SidebarResultsButton.tsx:138 — aria-disabled="true" on a has no effect; spans are not interactive elements so screen readers won't announce the disabled state. Use a or with pointer-events: none instead.

5. formatTimeWindowLine has an implicit fallthrough

EditableStopItem.tsx:263-265:

if (tw.kind === "by") return By ${label};
if (tw.kind === "at") return label;
return From ${label}; // catches anything that isn't "by" or "at"
Any unexpected kind value silently renders as "From …". Add an explicit if (tw.kind === "from") branch and a defensive fallback (e.g., return label or log an error) to avoid silently swallowing unknown values.

6. capacityUsed condition is redundant

vroomToRoutes.ts:214-216:

address?.deliveryQuantity && address.deliveryQuantity > 0
If deliveryQuantity is truthy (non-zero number), the > 0 check is redundant. Harmless but noisy — simplify to address?.deliveryQuantity || step.load?.[0] ?? 0.

Design / Architecture Concerns
7. formatContactLine duplicates recipientSummary logic

EditableStopItem.tsx:274-281 — formatContactLine in the results component duplicates the exact same name · phone join logic just extracted into recipientSummary.ts. The Stop type fields differ from AddressCard, but extracting a shared formatter that takes { name?: string; phone?: string } would eliminate the duplication.

8. Module-level mutable markerScaledSize singleton

Map.tsx:512 — let markerScaledSize at module scope is a lazy singleton that captures a google.maps.Size. This is fine in production, but during HMR the module won't re-evaluate and the cached size object could hold a stale reference if the Google Maps SDK reloads. Low risk for now, but worth a comment noting the intent.

9. Stop sequence numbers hardcoded to bg-cyan-500

Sidebar.tsx:941 — The sequence badge uses a hardcoded teal regardless of route accent color. This means all routes look identical at the stop level. If the hi-fi design calls for uniform teal, that's fine — but it's surprising given that the route header, polylines, and map pins all use accent. Confirm this is intentional.

10. Inline SVG icons defined locally instead of shared

PersonIcon, ClockIcon, NoteDocIcon, PackageIcon in EditableStopItem.tsx and the route/truck/chevron SVGs inline in Sidebar.tsx are all new one-off definitions. If the project has or plans a shared icon library, these belong there. At minimum, icons used in both components (like the route icon) should not be duplicated.

Minor / Style
b2-logo.png committed to public/ — Binary assets in the repo are fine, but confirm this is intentional and not a placeholder that should be loaded from a CDN.
"use client" added to SidebarResultsButton — Correct, since it uses usePathname. Good catch.
fill="currentColor" fix in SidebarResultsButton — Correct improvement; the previous fill="var(--edit-text-primary)" would have broken in contexts where the CSS variable isn't in scope.
recipient_phone CSV alias — Good addition; consistent with the existing alias pattern.
hasOptimizeResults.ts SSR guard (typeof window === "undefined") — Correct defensive check for Next.js server rendering.
Test Coverage
Area Status
recipientSummary / hasRecipientContact ✅ Well-covered
capacityUsed fallback logic ✅ Covered
addressCardToDeliveryInput recipient forwarding ✅ Covered
formatTime12h / formatDeliveryWindow ❌ Not tested
SidebarResultsButton routing state logic ❌ Not tested
hasOptimizeResults ❌ Not tested
PR Checklist Gaps
The submission has 8 unchecked validation items, notably format:check, build, manual testing on /results, and all mobile/backend checks. Given that this PR introduces navigation routing logic, a layout overhaul, and data model additions, the "Edit" → "Save edits" flow and custom map pin rendering at minimum should be manually verified before merge.

Summary
The code quality is generally solid — the utility extractions are clean, SVG icons are well-crafted, the deliveryWindowStart/End data flow is properly typed end-to-end, and new tests are meaningful. The two blocking concerns are the always-visible Save button and the stub Export button, both of which will confuse users immediately. The aria-disabled issue and formatTimeWindowLine fallthrough are smaller but worth fixing before merge.

[markboenigk]

All CI checks green. The latest commit addresses all bugs from the review — Save/Cancel gating, Export stub, isSidebarOpen dead code, aria-disabled on span, formatTimeWindowLine fallthrough, sequence badge accent colors, and the formatContactLine duplication.

Two small items still open but not blockers: the capacityUsed > 0 redundancy in vroomToRoutes.ts and the local SVG icon definitions. Let's track those separately.

Ready to merge once v6 lands and this is rebased onto main.