HI-FI: Edit Mode Layout and Banner Styling

[KesarSidhu]

Summary

• Updates Results edit-mode visuals to match hi-fi mock: teal “Save edits” control, route-card styling in edit mode, a centered map banner (“You are now in editing mode”), and a wider routes sidebar for card layout.
• Rebases onto upstream/main and keeps shared Results infrastructure (directions polylines, distance updates, route-colored map pins, edit nav rail).

Motivation

• The Results page edit state still used the older amber toggle and inset-shadow route cards, which did not match the hi-fi edit-mode design.
• Reviewers need clearer affordance when editing (banner + button label) and more horizontal room in the routes panel while adjusting stops and notes.

Changes

Frontend

• app/ui/src/app/results/page.tsx
  • Centered “You are now in editing mode” banner over the map when isEditMode is true (teal pill, non-interactive overlay).
  • Widened routes sidebar from w-72 to w-[26rem] for card layout room.
  • Hi-fi top bar (NAVBAR_V2_*) and left edit nav rail (EditSidebar, SidebarEditButton, SidebarResultsButton).
  • edit.module.css root tokens on <main> for shared design variables.
• app/ui/src/app/results/components/Sidebar.tsx
  • Replaced amber edit toggle with Edit / Save edits pill (#7BCFC2 when active).
  • Edit-mode route cards: teal border (#6CCBBE), white background; view mode keeps zinc styling.
  • Left accent strip via border-l-4 + routeColorHex; whitespace-nowrap on “Optimized Routes” title.
  • Footer branding block retained.
• app/ui/src/app/results/components/Map.tsx
  • Route-colored SVG teardrop pin markers (from stacked pin-colors work on this branch).
• app/ui/src/app/edit/components/layout/sidebar/SidebarResultsButton.tsx + app/ui/src/app/edit/utils/hasOptimizeResults.ts
  • Results nav link enabled when optimizeResults exists in sessionStorage (post-rebase wiring).

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

Manual: on /results, toggle Edit → confirm teal Save edits, map banner, wider sidebar, and teal-bordered route cards; toggle off → banner and edit styling clear.

Risk

• Low. UI-only changes on Results; no API or persistence changes.
• Edit-mode colors use fixed hex values (#7BCFC2, #6CCBBE) rather than CSS tokens; future token consolidation may need a small follow-up.
• Branch includes map pin styling from earlier stacked commits; behavior unchanged aside from pin appearance.

Rollout and Recovery

• Safe to merge and deploy with the frontend; no migrations or feature flags.
• Rollback: revert this PR to restore amber toggle, narrower sidebar, and prior route-card styling.
• No backend recovery steps required.

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.

[kirillakovalenko]

Code Review: PR #153 — HI-FI: Edit Mode Layout and Banner Styling

Author: Kesar Singh Sidhu | +302 / -184 across 5 files | State: Open

---

Overview

Converts the Results page edit-mode visuals from the old amber toggle/inset-shadow design to the hi-fi spec: teal "Save edits" pill, teal-bordered route cards, an edit-mode map banner, a wider sidebar (w-72 → w-[26rem]), and the shared edit-page nav rail. Also wires the Results nav button to activate when sessionStorage has optimize results, and upgrades map pins to route-colored SVG teardrops. UI-only — no API or persistence changes.

---

Correctness Issues

1. Map pin anchor offset is wrong (AdvancedMarkers path) Map.tsx — createRoutePinElement:

wrapper.style.transform = "translate(-50%, -50%)";

The comment says the goal is to place the SVG tip on the stop. translate(-50%, -50%) centers the element horizontally and vertically over the coordinate — so the circle body sits on the stop, not the tip. The fallback (non-AdvancedMarker) path gets this right with anchor: new google.maps.Point(MARKER_ICON_WIDTH / 2, MARKER_ICON_HEIGHT). The Advanced Markers path should use translate(-50%, -100%) instead to land the tip at the coordinate.

2. "Save" button always rendered regardless of pendingPinMove page.tsx — The old code showed both Cancel and Save only when pendingPinMove != null. Now Cancel is conditionally shown but Save is always rendered and always calls savePendingPinMove. When there's no pending move, clicking Save is a no-op at best or may surface an error. This appears to be an unintentional behavioral change rather than a design decision.

---

Code Quality

Good:

• hasOptimizeResults.ts is clean: SSR guard (typeof window === "undefined"), try/catch on JSON.parse, and only checks Array.isArray && length > 0 — exactly as much as needed.
• Disabled nav item now correctly uses <span aria-disabled="true"> instead of <Link href="#" aria-disabled> — the old approach didn't actually prevent keyboard/screen-reader navigation.
• fill="currentColor" on the SVG replaces the hardcoded CSS-variable reference — better practice.
• aria-hidden added to SVG, aria-current="page" for the active Results item.
• The markerSvgDataUrl XSS concern is mitigated: fillColor always comes from routeColorHex() (a predefined palette), which the comment calls out explicitly.

Concerns:

• isSidebarOpen is hardcoded to true and the toggle button is removed. If the hi-fi spec permanently removes sidebar toggling, the dead const isSidebarOpen = true and the transition-[width] class on the container are unused complexity. Clean them up or document the intent.
• The "Export" button in the navbar has no onClick — it's a non-functional stub with no TODO comment or disabled state to signal that to reviewers.
• Module-level mutable let markerScaledSize in Map.tsx is a singleton that persists across hot-reloads in dev. Low risk, but worth knowing.

---

Design / Token Debt

As called out in the PR description, #7BCFC2 and #6CCBBE are hardcoded in three places (Sidebar, page.tsx, SidebarResultsButton). The PR acknowledges this; just confirm there's a tracked follow-up task for token consolidation before these values proliferate further.

---

Validation Gaps

The PR has several unchecked validation items:

• npm run test and npm run build — these should be green before merge.
• No screenshots despite the PR checklist noting they're missing.

The risk is low (UI-only), but the test and build checks should not be skipped for a merge.

---

Summary

# | Severity | Issue -- | -- | -- 1 | Bug | AdvancedMarker pin anchor uses translate(-50%, -50%) — should be translate(-50%, -100%) to place tip at stop 2 | Bug | "Save" button always visible/clickable regardless of pendingPinMove state 3 | Minor | isSidebarOpen = true hardcoded with dead transition code; should be cleaned up or documented 4 | Minor | "Export" button is a non-functional stub with no affordance for that state 5 | Process | npm run test and npm run build not confirmed passing

Fix issues 1 and 2 before merging — everything else is polish or known tech debt.

Code Review: PR #153 — HI-FI: Edit Mode Layout and Banner Styling Author: Kesar Singh Sidhu | +302 / -184 across 5 files | State: Open

Overview
Converts the Results page edit-mode visuals from the old amber toggle/inset-shadow design to the hi-fi spec: teal "Save edits" pill, teal-bordered route cards, an edit-mode map banner, a wider sidebar (w-72 → w-[26rem]), and the shared edit-page nav rail. Also wires the Results nav button to activate when sessionStorage has optimize results, and upgrades map pins to route-colored SVG teardrops. UI-only — no API or persistence changes.

Correctness Issues

1. Map pin anchor offset is wrong (AdvancedMarkers path)
   Map.tsx — createRoutePinElement:

wrapper.style.transform = "translate(-50%, -50%)";
The comment says the goal is to place the SVG tip on the stop. translate(-50%, -50%) centers the element horizontally and vertically over the coordinate — so the circle body sits on the stop, not the tip. The fallback (non-AdvancedMarker) path gets this right with anchor: new google.maps.Point(MARKER_ICON_WIDTH / 2, MARKER_ICON_HEIGHT). The Advanced Markers path should use translate(-50%, -100%) instead to land the tip at the coordinate.

2. "Save" button always rendered regardless of pendingPinMove
   page.tsx — The old code showed both Cancel and Save only when pendingPinMove != null. Now Cancel is conditionally shown but Save is always rendered and always calls savePendingPinMove. When there's no pending move, clicking Save is a no-op at best or may surface an error. This appears to be an unintentional behavioral change rather than a design decision.

Code Quality
Good:

hasOptimizeResults.ts is clean: SSR guard (typeof window === "undefined"), try/catch on JSON.parse, and only checks Array.isArray && length > 0 — exactly as much as needed.
Disabled nav item now correctly uses instead of — the old approach didn't actually prevent keyboard/screen-reader navigation.
fill="currentColor" on the SVG replaces the hardcoded CSS-variable reference — better practice.
aria-hidden added to SVG, aria-current="page" for the active Results item.
The markerSvgDataUrl XSS concern is mitigated: fillColor always comes from routeColorHex() (a predefined palette), which the comment calls out explicitly.
Concerns:

isSidebarOpen is hardcoded to true and the toggle button is removed. If the hi-fi spec permanently removes sidebar toggling, the dead const isSidebarOpen = true and the transition-[width] class on the container are unused complexity. Clean them up or document the intent.
The "Export" button in the navbar has no onClick — it's a non-functional stub with no TODO comment or disabled state to signal that to reviewers.
Module-level mutable let markerScaledSize in Map.tsx is a singleton that persists across hot-reloads in dev. Low risk, but worth knowing.
Design / Token Debt
As called out in the PR description, #7BCFC2 and #6CCBBE are hardcoded in three places (Sidebar, page.tsx, SidebarResultsButton). The PR acknowledges this; just confirm there's a tracked follow-up task for token consolidation before these values proliferate further.

Validation Gaps
The PR has several unchecked validation items:

npm run test and npm run build — these should be green before merge.
No screenshots despite the PR checklist noting they're missing.
The risk is low (UI-only), but the test and build checks should not be skipped for a merge.

Summary

Severity Issue

1 Bug AdvancedMarker pin anchor uses translate(-50%, -50%) — should be translate(-50%, -100%) to place tip at stop
2 Bug "Save" button always visible/clickable regardless of pendingPinMove state
3 Minor isSidebarOpen = true hardcoded with dead transition code; should be cleaned up or documented
4 Minor "Export" button is a non-functional stub with no affordance for that state
5 Process npm run test and npm run build not confirmed passing
Fix issues 1 and 2 before merging — everything else is polish or known tech debt.

[markboenigk]

Checked the latest commit (6088dfe) — the two bugs Kirill marked as must-fix are done:

• ✅ Pin anchor corrected to translate(-50%, -100%)
• ✅ Save/Cancel now both conditional on pendingPinMove != null

Three items still need attention before this is mergeable:

1. Export button is a live no-op
It has no onClick, no disabled, and no aria-disabled — it's a fully interactive button that silently does nothing. Add disabled + aria-disabled="true" (and optionally title="Export coming soon") so the affordance matches the behavior.

2. Cancel and Save have identical styling
Both share the same border-[var(--edit-foreground)] hover:bg-black/5 className. Save should be visually distinct as the primary action (it was bg-amber-500 before — something equivalent for the new design system would work).

3. npm run test and npm run build still unchecked
These need to pass before merge.

Also worth a follow-up (not a blocker): const isSidebarOpen = true with the dead transition-[width] class on the container can be cleaned up once the hi-fi spec confirms the sidebar is permanently visible.

[markboenigk]

No new commits since 6088dfe — the three items from my last comment are still open.

Quick triage on severity:

• Export button and Save/Cancel styling are both minor polish — not blocking correctness.
• npm run build + npm run test is the only real gate. Given the JSX restructuring across 5 files, just need confirmation these pass before merge.

If the build is green, this is good to go. Everything else can be a follow-up.