From 0b5230d4ab73c607a636bbb25e898e539cc8318d Mon Sep 17 00:00:00 2001 From: SkateSpots Date: Sun, 27 Sep 2026 17:07:44 +0700 Subject: [PATCH] fix(directions): route map never rendered + dialog stacked under the spot sheet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RouteDialog was effectively broken — users got an empty dialog / 'no route'. - Map init: the Leaflet map lives in Radix Dialog content, which mounts AFTER the first effect pass, so the mapDivRef guard was null, the init effect bailed, and the map was never created (no route line, ever). Hold the node in state with a callback ref and add it to the effect deps. - Height: the global .leaflet-container{height:100%} override beats h-64; against a height-less parent it resolved to 0 (invisible map). Height now lives on the wrapper (relative h-64) and the map is h-full. - Stacking: from the map, Directions rendered the dialog inside the spot Sheet (two nested Radix modals; the sheet covered the route). The map now owns the RouteDialog at page level and closes the sheet first. - Routing transport: call OSRM directly (it sends CORS '*') with the cors proxy only as fallback; distinguish busy / no-route / unreachable errors and guard against NaN coordinates. --- AGENTS.md | 21 ++++++++++ src/components/RouteDialog.tsx | 47 ++++++++++++++-------- src/lib/routing.ts | 50 ++++++++++++++++++----- src/pages/MapPage.tsx | 73 +++++++++++++++++++++++++++++----- 4 files changed, 155 insertions(+), 36 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index fe8cf74..271f0b0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -199,6 +199,27 @@ session with **no other memory of this project**. So: **< 2 min old**; the blocking "Finding your location…" overlay shows only when there is no seed (with a seed it refines silently via a corner chip). The map "Near Me" button also flips the sort to `proximity`. + - ⚠️ **`RouteDialog` had a total-failure bug (fixed 2026-09-27), don't regress:** + (1) its Leaflet map is inside Radix Dialog content, which mounts AFTER the + component's first effect pass, so a `mapDivRef.current` guard was null, + the init effect bailed, and **the map was never created** (no route line + ever appeared). The node is now held in **state** (`const [mapDiv,setMapDiv]`) + with a callback ref, and `mapDiv` is in the effect deps so it runs when the + node actually mounts. + (2) Even once created, the global `.leaflet-container { height: 100% }` + override in `index.css` **beats `h-64`** on the map element; against a + height-less parent it resolved to 0 (invisible map). Height now lives on the + wrapper (`
`) and the map is `h-full`. + (3) On the map, Directions used to render the dialog INSIDE the spot `Sheet` + (two stacked Radix modals; the sheet covered the route). The map now owns + the dialog at page level (`routeTarget` state) and closes the sheet first. + - OSRM is called **directly** (`router.project-osrm.org` sends + `Access-Control-Allow-Origin: *`) with the `proxy.shakespeare.diy` CORS proxy + only as a fallback — faster, and origin/destination never leave OSRM unless the + direct call is blocked. Failures are distinguished: `Routing service busy` + (429/TooManyRequests), `No route found` (NoRoute / invalid coords), else + unreachable. Verify route rendering with `scripts/`-style CDP repros, not just + the smoke test (which only checks mount). - **`Layout` is a router layout** — `AppRouter` wraps every route (except `/amber-callback` and the `*` 404) in **one** `}>` with an ``, so the chrome (header, nav, notifications, Sasquatch) diff --git a/src/components/RouteDialog.tsx b/src/components/RouteDialog.tsx index f2a0bfd..b47fe1c 100644 --- a/src/components/RouteDialog.tsx +++ b/src/components/RouteDialog.tsx @@ -20,6 +20,8 @@ import { formatDuration, TRAVEL_MODES, TRANSIT_ATTRIBUTION, + ROUTE_ERR_BUSY, + ROUTE_ERR_NO_ROUTE, type TravelMode, type RouteResult, } from '@/lib/routing'; @@ -89,7 +91,12 @@ export function RouteDialog({ destination, destinationLabel = 'Destination', }: RouteDialogProps) { - const mapDivRef = useRef(null); + // The map container is inside Radix Dialog content, which mounts AFTER this + // component's first effect pass. A plain useRef is therefore still null when + // the init effect runs, and — because the effect's deps never changed again — + // the map was NEVER created (empty dialog, no route line). Holding the node in + // state makes the effect re-run the moment it actually mounts. + const [mapDiv, setMapDiv] = useState(null); const mapRef = useRef(null); const routeLayersRef = useRef(null); const originMarkerRef = useRef(null); @@ -149,9 +156,9 @@ export function RouteDialog({ // ─── Initialize the Leaflet map once the dialog content is mounted ───────── useEffect(() => { - if (!open || !mapDivRef.current || mapRef.current || !destination) return; + if (!open || !mapDiv || mapRef.current || !destination) return; - const map = L.map(mapDivRef.current, { + const map = L.map(mapDiv, { center: [destination.lat, destination.lng], zoom: 14, scrollWheelZoom: true, @@ -188,7 +195,7 @@ export function RouteDialog({ originMarkerRef.current = null; destMarkerRef.current = null; }; - }, [open, destination]); + }, [open, mapDiv, destination]); // ─── Draw / update the origin marker when we have the user's location ────── useEffect(() => { @@ -268,15 +275,20 @@ export function RouteDialog({ } catch (err) { if (cancelled) return; if (err instanceof DOMException && err.name === 'AbortError') return; - setError( - mode === 'transit' - ? err instanceof Error && err.message === 'No transit route found' + const msg = err instanceof Error ? err.message : ''; + if (mode === 'transit') { + setError( + msg === 'No transit route found' ? 'No public-transport route found here — this area may not publish open timetables.' - : "Couldn't reach the transit service. Check your connection and try again." - : err instanceof Error && err.message === 'No route found' - ? 'No route found for this mode. Try another mode.' - : "Couldn't reach the routing service. Check your connection and try again.", - ); + : "Couldn't reach the transit service. Check your connection and try again.", + ); + } else if (msg === ROUTE_ERR_BUSY) { + setError('The routing service is busy right now — try again in a moment.'); + } else if (msg === ROUTE_ERR_NO_ROUTE) { + setError('No route for this mode — the destination may be off the road network. Try another mode.'); + } else { + setError("Couldn't reach the routing service. Check your connection and try again."); + } } finally { if (!cancelled) setRouting(false); } @@ -324,11 +336,14 @@ export function RouteDialog({ ))}
- {/* Map */} -
+ {/* Map. The height lives on the WRAPPER: the global + `.leaflet-container { height: 100% }` override beats a `h-64` on the + map element itself, and against a height-less parent that resolved to + 0 — a fully invisible map with no route line. */} +
diff --git a/src/lib/routing.ts b/src/lib/routing.ts index 34bc9ca..b27c461 100644 --- a/src/lib/routing.ts +++ b/src/lib/routing.ts @@ -98,9 +98,44 @@ interface OsrmResponse { }>; } +/** Error message constants the UI branches on (kept in sync with RouteDialog). */ +export const ROUTE_ERR_NO_ROUTE = 'No route found'; +export const ROUTE_ERR_BUSY = 'Routing service busy'; + +function isPoint(p: { lat?: number; lng?: number } | null | undefined): boolean { + return !!p && Number.isFinite(p.lat) && Number.isFinite(p.lng); +} + +/** + * Call OSRM and parse the JSON. + * + * OSRM sends `Access-Control-Allow-Origin: *`, so we call it DIRECTLY first — + * one less third-party hop (faster, and the origin/destination never pass + * through a third party). Only if the direct call fails do we retry through the + * CORS proxy (some networks/WebViews block the direct request). + */ +async function fetchOsrm(url: string, signal?: AbortSignal): Promise { + try { + const res = await fetch(url, { signal }); + if (res.ok) return (await res.json()) as OsrmResponse; + if (res.status === 429) throw new Error(ROUTE_ERR_BUSY); + // Any other non-OK (400/404/500…) is worth one proxy retry. + } catch (e) { + if ((e as Error)?.name === 'AbortError') throw e; + if ((e as Error)?.message === ROUTE_ERR_BUSY) throw e; + // fall through to the proxy + } + + const proxied = `${CORS_PROXY}${encodeURIComponent(url)}`; + const res = await fetch(proxied, { signal }); + if (res.status === 429) throw new Error(ROUTE_ERR_BUSY); + if (!res.ok) throw new Error(`Routing service returned ${res.status}`); + return (await res.json()) as OsrmResponse; +} + /** * Fetch a route between two points for a given travel mode using OSRM. - * Throws on network error or when OSRM returns no route. + * Throws `ROUTE_ERR_BUSY`, `ROUTE_ERR_NO_ROUTE`, or a network error. */ export async function fetchRoute( from: { lat: number; lng: number }, @@ -110,19 +145,16 @@ export async function fetchRoute( ): Promise { const { profile } = travelModeMeta(mode); if (!profile) throw new Error('Not a road routing mode'); // transit uses fetchTransitRoute + // Never send NaN/undefined to OSRM — that surfaces as a confusing "no route". + if (!isPoint(from) || !isPoint(to)) throw new Error(ROUTE_ERR_NO_ROUTE); // OSRM wants lon,lat;lon,lat const coords = `${from.lng},${from.lat};${to.lng},${to.lat}`; const target = `${OSRM_BASE}/${profile}/${coords}?overview=full&geometries=geojson`; - const url = `${CORS_PROXY}${encodeURIComponent(target)}`; - const res = await fetch(url, { signal }); - if (!res.ok) { - throw new Error(`Routing service returned ${res.status}`); - } - - const data = (await res.json()) as OsrmResponse; + const data = await fetchOsrm(target, signal); + if (data.code === 'TooManyRequests') throw new Error(ROUTE_ERR_BUSY); if (data.code !== 'Ok' || !data.routes || data.routes.length === 0) { - throw new Error('No route found'); + throw new Error(ROUTE_ERR_NO_ROUTE); } const route = data.routes[0]; diff --git a/src/pages/MapPage.tsx b/src/pages/MapPage.tsx index a24fb76..05d52fe 100644 --- a/src/pages/MapPage.tsx +++ b/src/pages/MapPage.tsx @@ -347,7 +347,23 @@ function StashDrawer() { // In-app directions — opens the same RouteDialog used on the detail pages. // Deliberately NO external map handoff: routing stays inside SkateSpots. -function DirectionsButton({ lat, lon, label }: { lat: string; lon: string; label: string }) { +function DirectionsButton({ + lat, + lon, + label, + onDirections, +}: { + lat: string; + lon: string; + label: string; + /** + * When provided, the PARENT owns the RouteDialog. Rendering a Radix Dialog + * inside the spot's Radix Sheet stacked two modals (the sheet covered the + * route), so the map passes this and renders the dialog itself; the sheet + * then closes and the route gets the whole screen. + */ + onDirections?: (target: { lat: number; lng: number; label: string }) => void; +}) { const [open, setOpen] = useState(false); const destination = useMemo(() => { const la = parseFloat(lat); @@ -360,24 +376,38 @@ function DirectionsButton({ lat, lon, label }: { lat: string; lon: string; label - + {!onDirections && ( + + )} ); } // Bottom sheet shown when a map marker is tapped -function SpotBottomSheet({ spot, onClose }: { spot: SkateSpot; onClose: () => void }) { +function SpotBottomSheet({ + spot, + onClose, + onDirections, +}: { + spot: SkateSpot; + onClose: () => void; + onDirections: (target: { lat: number; lng: number; label: string }) => void; +}) { const navigate = useNavigate(); const author = useAuthor(spot.pubkey); const metadata = author.data?.metadata; @@ -480,7 +510,7 @@ function SpotBottomSheet({ spot, onClose }: { spot: SkateSpot; onClose: () => vo const label = spot.title; return (
- +
); })()} @@ -534,6 +564,9 @@ export default function MapPage() { const [searchQuery, setSearchQuery] = useState(''); const [selectedCategories, setSelectedCategories] = useState([]); const [selectedSpot, setSelectedSpot] = useState(null); + // Directions target lives at page level (NOT inside the spot Sheet) so the + // RouteDialog is never nested inside another modal. + const [routeTarget, setRouteTarget] = useState<{ lat: number; lng: number; label: string } | null>(null); const [weatherEnabled, setWeatherEnabled] = useState(false); const [weatherFilter, setWeatherFilter] = useState<'all' | 'good' | 'perfect'>('all'); // Smart WoT default: @@ -1914,9 +1947,27 @@ export default function MapPage() { className="h-auto max-h-[55vh] rounded-t-2xl px-0 pb-0 bg-black/95 border-t border-white/10" > {selectedSpot?.title ?? 'Spot details'} - {selectedSpot && setSelectedSpot(null)} />} + {selectedSpot && ( + setSelectedSpot(null)} + onDirections={(target) => { + // Close the sheet first — the route dialog takes over. + setSelectedSpot(null); + setRouteTarget(target); + }} + /> + )} + + {/* In-app route — page-level so it never stacks inside the spot Sheet. */} + { if (!open) setRouteTarget(null); }} + destination={routeTarget ? { lat: routeTarget.lat, lng: routeTarget.lng } : null} + destinationLabel={routeTarget?.label ?? 'Spot'} + /> ); }