fix: detour calculation, loading spinner, station waypoints#16
fix: detour calculation, loading spinner, station waypoints#16
Conversation
- Use symmetric 3% offset for Valhalla detour calculation instead of 1% forward-only, fixing inflated detour times (93min → realistic) - Show loading spinner while corridor stations are being fetched - Clicking a station in sidebar adds it as a route waypoint/leg, clicking another replaces the previous station leg
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetour logic now uses a symmetric station-centered index window and uses the computed exitIndex for original-segment fraction; HomeClient/MapView/SearchPanel gained stations-loading state/callbacks and UI feedback; SearchPanel adds station-leg waypoint insertion, waypoint-routing versioning, and related helpers; added station translation keys. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/map/map-view.tsx (1)
116-138:⚠️ Potential issue | 🟠 MajorLoading state can get stuck on aborted corridor fetches.
When the request aborts, control returns before clearing loading. That can leave the sidebar spinner pinned until a later successful fetch.
🐛 Proposed fix
const fetchAllRouteStations = useCallback( async (fuel: FuelType, routeList: Route[]) => { if (corridorAbortRef.current) corridorAbortRef.current.abort(); const controller = new AbortController(); corridorAbortRef.current = controller; onStationsLoadingChange?.(true); try { const km = corridorKmRef.current; const results = await Promise.all( routeList.map((r) => fetch("/api/route-stations", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ geometry: r.geometry, fuel, corridorKm: km }), signal: controller.signal, }).then((res) => (res.ok ? res.json() as Promise<StationsGeoJSONCollection> : EMPTY_COLLECTION)), ), ); const total = results.reduce((sum, r) => sum + r.features.length, 0); const unique = new Set(results.flatMap((r) => r.features.map((f) => f.properties.id))).size; console.log(`[map] Route corridors: ${results.map((r) => r.features.length).join("+")} = ${total} stations (${unique} unique) for ${fuel}`); setCorridorPerRoute(results); - onStationsLoadingChange?.(false); } catch (err) { - if (err instanceof DOMException && err.name === "AbortError") return; - console.error("[map] Failed to fetch route stations:", err); - onStationsLoadingChange?.(false); + if (!(err instanceof DOMException && err.name === "AbortError")) { + console.error("[map] Failed to fetch route stations:", err); + } + } finally { + if (corridorAbortRef.current === controller) { + onStationsLoadingChange?.(false); + } } }, [onStationsLoadingChange], );} else { // Abort any in-flight corridor fetch so stale results don't leak back if (corridorAbortRef.current) { corridorAbortRef.current.abort(); corridorAbortRef.current = null; } + onStationsLoadingChange?.(false); setCorridorPerRoute([]); fetchStations(selectedFuel); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/map/map-view.tsx` around lines 116 - 138, The abort path in the try/catch returns before clearing the loading indicator, so update the catch for AbortError to call onStationsLoadingChange?.(false) before returning; specifically, in the catch block that checks "if (err instanceof DOMException && err.name === 'AbortError')" call onStationsLoadingChange?.(false) (and any necessary cleanup like abort controller handling) prior to the early return so the spinner is always cleared when fetches are aborted; locate this logic around the onStationsLoadingChange, controller.signal usage and the catch handling for AbortError in the function where setCorridorPerRoute and the fetch Promise.all are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/route-detour/route.ts`:
- Around line 64-71: halfOffset’s fixed 30-point minimum skews small routes and
the degenerate case currently returns a misleading zero; change the halfOffset
computation to scale with route length (e.g., use Math.max(1,
Math.round(numCoords * 0.03)) instead of Math.max(30,...)) so short polylines
still get a meaningful window, and modify the degenerate-window branch (the
exitIndex === rejoinIndex check that currently returns { id: s.id, detourMin: 0
}) to indicate an uncomputable detour (for example return { id: s.id, detourMin:
null } or skip this station) rather than returning 0 so callers can distinguish
real zero detours from invalid windows.
In `@src/components/home-client.tsx`:
- Line 44: Replace the local stationsLoading state in the HomeClient component
with a shared Zustand UI store: install zustand, create a UI store (e.g.,
useUIStore) that exposes stationsLoading:boolean and
setStationsLoading(fn|value), move initialization and setters out of
home-client.tsx, then update HomeClient, MapView, and SearchPanel to import
useUIStore and read/write stationsLoading from that store instead of passing it
as props or using setStationsLoading from home-client; ensure you remove the
useState declaration for stationsLoading in home-client and update any prop
typings where MapView/SearchPanel previously received stationsLoading or
setStationsLoading.
---
Outside diff comments:
In `@src/components/map/map-view.tsx`:
- Around line 116-138: The abort path in the try/catch returns before clearing
the loading indicator, so update the catch for AbortError to call
onStationsLoadingChange?.(false) before returning; specifically, in the catch
block that checks "if (err instanceof DOMException && err.name ===
'AbortError')" call onStationsLoadingChange?.(false) (and any necessary cleanup
like abort controller handling) prior to the early return so the spinner is
always cleared when fetches are aborted; locate this logic around the
onStationsLoadingChange, controller.signal usage and the catch handling for
AbortError in the function where setCorridorPerRoute and the fetch Promise.all
are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac847e02-87ab-4d67-a51a-f9b4d5717798
📒 Files selected for processing (5)
src/app/api/route-detour/route.tssrc/components/home-client.tsxsrc/components/map/map-view.tsxsrc/components/search/search-panel.tsxsrc/lib/i18n.tsx
| const [selectedStationId, setSelectedStationId] = useState<string | null>(null); | ||
| const [maxPrice, setMaxPrice] = useState<number | null>(null); | ||
| const [maxDetour, setMaxDetour] = useState<number | null>(null); | ||
| const [stationsLoading, setStationsLoading] = useState(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where stationsLoading is defined/prop-drilled and whether Zustand is used for this concern.
rg -n --type=ts --type=tsx '\bstationsLoading\b|\bsetStationsLoading\b|onStationsLoadingChange' src
rg -n --type=ts --type=tsx 'from ["'"'"']zustand["'"'"']|create\(' srcRepository: GeiserX/Pumperly
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Search for stationsLoading usage (fixed syntax)
rg -n 'stationsLoading|setStationsLoading|onStationsLoadingChange' src --type ts
# Search for Zustand imports and store creation
rg -n "from\s+['\"]zustand['\"]|create(" src --type ts
# Also check for .tsx files specifically
find src -name "*.tsx" -type f | head -20Repository: GeiserX/Pumperly
Length of output: 2075
🏁 Script executed:
#!/bin/bash
# Search for Zustand imports and store creation (fixed regex)
rg -n "from ['\"]zustand|create\s*\(" src --type ts
# Check home-client.tsx structure around the state usage
sed -n '40,50p' src/components/home-client.tsx
# Check the prop drilling at lines 292 and 306
sed -n '285,310p' src/components/home-client.tsx
# Check if there's an existing store/zustand directory
find src -path "*store*" -o -path "*zustand*" | head -20Repository: GeiserX/Pumperly
Length of output: 1687
🏁 Script executed:
#!/bin/bash
# Search for Zustand more broadly
rg -n "zustand" src --type ts
# Look for store files
find src -type f -name "*store*" -o -name "*Store*"
# Check if there's a hooks directory with custom hooks
find src -type d -name "hooks"
# Search for any existing UI state patterns
rg -n "export const create|export const use" src --type ts | grep -i store
# Get the full home-client.tsx to understand the component structure
wc -l src/components/home-client.tsxRepository: GeiserX/Pumperly
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Get the full home-client.tsx file
cat -n src/components/home-client.tsx
# Search for zustand imports anywhere in the entire src
rg "zustand" src
# Check package.json for zustand dependency
grep -i zustand package.jsonRepository: GeiserX/Pumperly
Length of output: 13701
🏁 Script executed:
#!/bin/bash
# Check package.json for zustand and other state management libraries
cat package.json | grep -A 5 -B 5 "zustand\|pinia\|redux\|recoil"
# Also check all dependencies
cat package.json | grep -E '"(zustand|dependencies|devDependencies)"' -A 20Repository: GeiserX/Pumperly
Length of output: 878
🏁 Script executed:
#!/bin/bash
# Verify MapView and SearchPanel are indeed siblings by checking home-client structure
grep -n "MapView\|SearchPanel" src/components/home-client.tsx | head -10
# Check if there are any other uses of stationsLoading outside these files
rg -c "stationsLoading" src --type tsRepository: GeiserX/Pumperly
Length of output: 302
Use Zustand for stationsLoading state instead of prop-drilling across MapView and SearchPanel.
This state is shared between sibling components and should be in a global store per the coding guideline for src/**/*.{ts,tsx}. Zustand is not currently in dependencies—install it first, then create a UI store for this state and any other shared UI concerns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/home-client.tsx` at line 44, Replace the local stationsLoading
state in the HomeClient component with a shared Zustand UI store: install
zustand, create a UI store (e.g., useUIStore) that exposes
stationsLoading:boolean and setStationsLoading(fn|value), move initialization
and setters out of home-client.tsx, then update HomeClient, MapView, and
SearchPanel to import useUIStore and read/write stationsLoading from that store
instead of passing it as props or using setStationsLoading from home-client;
ensure you remove the useState declaration for stationsLoading in home-client
and update any prop typings where MapView/SearchPanel previously received
stationsLoading or setStationsLoading.
…tion order - Respect MAX_WAYPOINTS cap when adding station leg (skip if full) - Clear waypoints when destination is cleared to avoid stale intermediates - Clear stationsLoading on aborted corridor fetches (prevents stuck spinner) - Insert station leg at correct position based on routeFraction instead of always appending at end
- Guard corridor fetch state writes with controller identity check so superseded requests never hide the spinner or write stale data - Clear route, waypoints, and stations when user types in destination field during active route (not just when clicking X) - Project manual waypoints onto route geometry for real fractions instead of assuming even spacing when inserting station legs
- Reset stationsLoading to false in the routes-cleared branch so a stale true never leaks across route attempts - Clear active route when user edits a waypoint text field, matching the dest-change cleanup pattern
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/search/search-panel.tsx (1)
791-822: Euclidean projection on geographic coordinates.This uses planar Euclidean distance in lon/lat space, which distorts at higher latitudes. For waypoint insertion ordering, this is acceptable since relative ordering is usually preserved. Consider noting this limitation if precision becomes critical.
📝 Optional: Add a brief comment about coordinate system
/** Project a point onto a LineString and return its fraction (0–1) along the line. */ +// Note: Uses planar Euclidean projection; acceptable for waypoint ordering but not geodesically accurate. function projectOntoRoute(point: [number, number], coords: [number, number][]): number {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/search/search-panel.tsx` around lines 791 - 822, The function projectOntoRoute uses planar Euclidean calculations on raw lon/lat coordinates which can produce distorted distances at higher latitudes; add a clear comment above projectOntoRoute stating that the projection and distance computations are in simple Cartesian lon/lat space (not geodesic), that this may introduce error at high latitudes or long distances, and that if strict geodesic accuracy is required the function should be replaced with a geodesic-aware projection (e.g., using haversine/Great Circle math or projecting to a suitable CRS) — reference projectOntoRoute and mention segLens/totalLen/frac logic so reviewers can find the relevant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/search/search-panel.tsx`:
- Around line 791-822: The function projectOntoRoute uses planar Euclidean
calculations on raw lon/lat coordinates which can produce distorted distances at
higher latitudes; add a clear comment above projectOntoRoute stating that the
projection and distance computations are in simple Cartesian lon/lat space (not
geodesic), that this may introduce error at high latitudes or long distances,
and that if strict geodesic accuracy is required the function should be replaced
with a geodesic-aware projection (e.g., using haversine/Great Circle math or
projecting to a suitable CRS) — reference projectOntoRoute and mention
segLens/totalLen/frac logic so reviewers can find the relevant code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4686b4b-4460-4b60-9d96-92099b4fa4e5
📒 Files selected for processing (2)
src/components/map/map-view.tsxsrc/components/search/search-panel.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/map/map-view.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/search/search-panel.tsx`:
- Line 46: The isStationLeg flag is not being cleared when a user edits a
station row, which leaves the pin icon stuck and causes subsequent station
clicks to delete the edited waypoint; update the row-editing handlers to clear
that flag: in the function(s) that handle row edits/changes and saves (e.g., the
onRowChange/onEditRow/saveRow handlers and any code that converts a station to a
manual waypoint), explicitly set row.isStationLeg = false (or call
setIsStationLeg(false) if using state) before updating the waypoint list so the
UI pin and future click logic behave correctly; ensure you apply this change in
the same places referenced in the comment where rows are edited/converted so the
flag is consistently reset.
- Around line 203-206: The panel unconditionally sets phase to "destination"
after clearing a route, which prevents the UI from returning to "route" when a
waypoint is reselected; in the waypoint selection handler where onClearRoute()
and setPhase(...) are called, change the logic to restore phase to "route" when
the user reselects or edits an existing waypoint (or when the previous phase was
"route"), and only set phase to "destination" for true new-destination flows —
update the code around the phase, onClearRoute(), and setPhase(...) calls so the
waypoint-select path sets setPhase("route") appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 249196b0-2dce-4339-a529-5d2c207f30f9
📒 Files selected for processing (2)
src/components/map/map-view.tsxsrc/components/search/search-panel.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/map/map-view.tsx
- Replace side-effect-in-updater pattern with wpRouteVersion counter and useEffect, so route recalc happens after state commits cleanly (fixes double-fire in React Strict Mode) - Show transient amber banner when station leg can't be added because manual waypoints already fill the 5-waypoint cap - Add stations.maxStops i18n key to all 16 locales
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/search/search-panel.tsx (2)
210-216:⚠️ Potential issue | 🟠 Major
isStationLegnot cleared when editing a station waypoint.When a user manually edits a station-leg row,
isStationLegstaystrue. The pin icon persists, and clicking another station will delete this now-manual waypoint.Proposed fix
const handleWaypointChange = useCallback((wpId: number, val: string) => { - setWaypoints((prev) => prev.map((wp) => (wp.id === wpId ? { ...wp, text: val, location: null } : wp))); + setWaypoints((prev) => prev.map((wp) => ( + wp.id === wpId + ? { ...wp, text: val, location: null, isStationLeg: false } + : wp + ))); if (phase === "route") { onClearRoute(); setPhase("destination"); } }, [phase, onClearRoute]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/search/search-panel.tsx` around lines 210 - 216, The bug is that editing a station waypoint doesn't clear its isStationLeg flag; update handleWaypointChange so when mapping waypoints (in the setWaypoints call inside handleWaypointChange) you also set isStationLeg: false for the edited waypoint (the one with id === wpId) so the pin icon and station-specific behavior are removed; keep the existing clearing of location and the phase/onClearRoute logic unchanged and reference the handleWaypointChange and setWaypoints usages to locate the change.
170-179:⚠️ Potential issue | 🟠 MajorPhase remains
"destination"after waypoint reselection — station list stays hidden.When a user edits a waypoint (triggering
handleWaypointChangewhich setsphaseto"destination"), then selects a new result here, the route recalculates butphaseis never restored to"route". The station list and spinner remain hidden.Proposed fix
const handleWaypointSelect = useCallback( (wpId: number, result: PhotonResult) => { const loc: Location = { label: formatResult(result), coordinates: result.coordinates }; setWaypoints((prev) => prev.map((wp) => (wp.id === wpId ? { ...wp, text: formatResult(result), location: loc } : wp)), ); + setPhase("route"); setWpRouteVersion((v) => v + 1); }, [], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/search/search-panel.tsx` around lines 170 - 179, handleWaypointSelect updates the waypoint but never restores the UI phase, so after handleWaypointChange sets phase to "destination" the UI stays hidden; fix by calling setPhase("route") (or the appropriate phase used to show station list/spinner) inside handleWaypointSelect after updating waypoints and before/after setWpRouteVersion, ensuring the component returns to the routing state; reference functions/state: handleWaypointSelect, handleWaypointChange, setWaypoints, setWpRouteVersion, setPhase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/search/search-panel.tsx`:
- Around line 306-314: The updater passed to setWaypoints currently contains
side effects (calls to setStationLegMsg and setTimeout) and a cap check against
MAX_WAYPOINTS; move the cap check and the user-facing message handling out of
the setWaypoints updater so the updater remains pure. Before calling
setWaypoints, derive the candidate withoutOld (e.g., filter current waypoints or
a ref to waypoints, removing isStationLeg), check if withoutOld.length >=
MAX_WAYPOINTS and if so call setStationLegMsg(...) and setTimeout(...),
otherwise call setWaypoints(prev => prev.filter(wp => !wp.isStationLeg) /* or
appropriate pure update */). Ensure only pure state updates occur inside the
setWaypoints callback and that any reads of current waypoints use stable state
or a ref to avoid stale values.
---
Duplicate comments:
In `@src/components/search/search-panel.tsx`:
- Around line 210-216: The bug is that editing a station waypoint doesn't clear
its isStationLeg flag; update handleWaypointChange so when mapping waypoints (in
the setWaypoints call inside handleWaypointChange) you also set isStationLeg:
false for the edited waypoint (the one with id === wpId) so the pin icon and
station-specific behavior are removed; keep the existing clearing of location
and the phase/onClearRoute logic unchanged and reference the
handleWaypointChange and setWaypoints usages to locate the change.
- Around line 170-179: handleWaypointSelect updates the waypoint but never
restores the UI phase, so after handleWaypointChange sets phase to "destination"
the UI stays hidden; fix by calling setPhase("route") (or the appropriate phase
used to show station list/spinner) inside handleWaypointSelect after updating
waypoints and before/after setWpRouteVersion, ensuring the component returns to
the routing state; reference functions/state: handleWaypointSelect,
handleWaypointChange, setWaypoints, setWpRouteVersion, setPhase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a78dd5a2-94b3-4ab6-a8a2-be2c0e473c85
📒 Files selected for processing (2)
src/components/search/search-panel.tsxsrc/lib/i18n.tsx
✅ Files skipped from review due to trivial changes (1)
- src/lib/i18n.tsx
…on edit - wpRouteVersion effect now restores phase to 'route' when origin and destination exist, so reselecting a waypoint after editing brings the route back - handleWaypointSelect and handleWaypointChange clear isStationLeg so edited station-leg rows become normal waypoints and won't be removed by a subsequent station click
…dges - Guard handleRoute state writes with controller identity check to prevent stale route responses overwriting newer ones - Rewrite detour calculation to use cumulative path lengths instead of vertex indices: window is now 3% of route length (not vertex count), and baseline uses length-based fractions matching PostGIS ST_LineLocatePoint output - Unknown detours during progressive loading now pass maxDetour filter, sort to end in detour mode, and are excluded from shortestDetour/balanced badge computation - Move max-stops banner logic out of setWaypoints updater to keep it pure under React replayed updates
…eedback - Include stations with null price (EV chargers) in detour pipeline, sidebar list, sorting, and badges — price filter passes null-price stations, price sort sends them to end, avgPrice/cheapestId only considers priced stations - Pass detourMap to MapView and enrich corridor features before filtering, so maxDetour hides the same stations on map and sidebar - Add routeError state: handleRoute sets error on !res.ok, empty response, or catch; SearchPanel shows red banner and rolls phase back to destination; handleClearRoute resets error
…e error banner - Widen detour window when sparse geometry collapses exit/rejoin to same vertex - Store -1 sentinel for failed detour lookups; exclude from filters/badges/sort - Mark non-OK detour batches as failed instead of silently skipping - Allow Enter to retry route after failure (destination already resolved) - Clear stale routeError on input edits in post-failure state
…, warn on corridor fetch fail
…mpty states, full i18n - Clear primaryStations when new route arrives to prevent detour effect from pairing new route geometry with previous corridor stations - Cap routeCoordinates at 3000 entries in route-detour schema - Separate stations.noStations from stations.empty for zero-corridor case - Localize route error, no-route, no-results strings across all 16 locales
- Replace new Function() with vm.runInNewContext() for Nuxt 2 payload parsing — sandboxed context with no globals and 1s timeout - StatsDropdown now shows error state on fetch failure instead of rendering an empty panel
Summary
Test plan
Summary by CodeRabbit
New Features
UI/UX Improvements
Behavior Changes