fix: Valhalla detour baseline + preserve stations on click#17
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughReplaces baseline-from-request with a second Valhalla call run in parallel for baseline duration; removes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/api/route-detour/route.ts (1)
125-138: Race condition: concurrentresults.push()from multiple workers.Multiple async workers call
results.push(result)concurrently. While JS is single-threaded, theawaityields control, and if workers resume in interleaved order, push operations could theoretically interleave. In practice this works in Node.js, but it's fragile. Consider using the station index or collecting per-worker arrays.Safer pattern
- const results: DetourResult[] = []; - const queue = [...stations]; + const results: DetourResult[] = new Array(stations.length); + let nextIdx = 0; async function processStation( s: z.infer<typeof stationSchema>, + idx: number, ): Promise<DetourResult> { // ... existing logic ... } const workers: Promise<void>[] = []; for (let i = 0; i < CONCURRENCY; i++) { workers.push( (async () => { - while (queue.length > 0) { - const station = queue.shift()!; - const result = await processStation(station); - results.push(result); + while (nextIdx < stations.length) { + const idx = nextIdx++; + const station = stations[idx]; + results[idx] = await processStation(station, idx); } })(), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/route-detour/route.ts` around lines 125 - 138, The current loop in which multiple async workers call results.push(result) (inside the CONCURRENCY worker tasks that await processStation and shift from queue) can race; change each worker to accumulate into a local array (e.g., localResults) instead of mutating the shared results, have each worker return that array (so workers become Promise<MyType[]> rather than Promise<void>), then await Promise.all(workers) and flatten/concat the array-of-arrays into the shared results once all workers complete; reference the existing symbols queue, processStation, results, workers, and CONCURRENCY to locate and update the code accordingly.
🤖 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 97-116: The code calls getRouteDuration in Promise.all to compute
detourDuration and baselineDuration but does not catch exceptions (e.g.,
TimeoutError or network errors), so a single failing call can abort the whole
batch; wrap the Promise.all(...) invocation that sets detourDuration and
baselineDuration in a try-catch around the await, and in the catch return the
fallback object { id: s.id, detourMin: -1 } (same as the null-check path);
ensure you still keep the existing null-check for
detourDuration/baselineDuration but handle thrown errors by catching them and
returning the fallback immediately.
---
Nitpick comments:
In `@src/app/api/route-detour/route.ts`:
- Around line 125-138: The current loop in which multiple async workers call
results.push(result) (inside the CONCURRENCY worker tasks that await
processStation and shift from queue) can race; change each worker to accumulate
into a local array (e.g., localResults) instead of mutating the shared results,
have each worker return that array (so workers become Promise<MyType[]> rather
than Promise<void>), then await Promise.all(workers) and flatten/concat the
array-of-arrays into the shared results once all workers complete; reference the
existing symbols queue, processStation, results, workers, and CONCURRENCY to
locate and update the code accordingly.
🪄 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: d92868e8-d2e4-4cbf-ac30-e909d325e8f1
📒 Files selected for processing (4)
src/app/api/route-detour/route.tssrc/components/home-client.tsxsrc/components/map/map-view.tsxsrc/components/search/search-panel.tsx
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/search/search-panel.tsx (1)
351-357:⚠️ Potential issue | 🟡 MinorPreview legs still count toward
MAX_WAYPOINTS.This branch excludes
isStationLegfrommanualCount, butaddWaypoint()and the add-waypoint render guard still usewaypoints.length. With 4 manual stops + 1 selected station, the UI hides the add action one stop early.🤖 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 351 - 357, The cap check is inconsistent: this branch computes manualCount by excluding isStationLeg but addWaypoint and the add-waypoint render guard use waypoints.length, causing the add action to hide early; change the check to use the same total count as the rest of the code (e.g., replace manualCount with totalCount = waypoints.length or create a shared helper like getWaypointCount that returns the full waypoint count) and update both this check and addWaypoint/add-waypoint render guard to call that same helper so station-preview legs are consistently counted against MAX_WAYPOINTS.
🤖 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 97-106: The clear logic in search-panel.tsx only calls
onClearStationLeg(), but onClearStationLeg in home-client.tsx merely nulls
stationLegRoutes while an in-flight handleRoute() request can later overwrite
the preview; fix by making onClearStationLeg cancel/ignore pending station-leg
requests (e.g., add an AbortController or a requestId flag used by handleRoute)
and ensure handleRoute checks the abort/requestId before writing
stationLegRoutes; update the clear-sites (both the useEffect for
selectedStationId and the click-X handler) to call the enhanced
onClearStationLeg so any in-flight station-leg preview is aborted or ignored.
- Around line 141-145: When building the waypoint coordinates passed to onRoute
(the wpCoords used by calculateRoute/route callback), filter out preview-only
station legs by excluding WaypointEntry items with isStationLeg truthy before
mapping to .location.coordinates; i.e., change the wpCoords formation in the
anonymous route callback (and the analogous code around lines 196-198) to
.filter(wp => wp.location != null && !wp.isStationLeg).map(wp =>
wp.location!.coordinates) so onRoute receives only real stops for base-route
recalcs.
---
Outside diff comments:
In `@src/components/search/search-panel.tsx`:
- Around line 351-357: The cap check is inconsistent: this branch computes
manualCount by excluding isStationLeg but addWaypoint and the add-waypoint
render guard use waypoints.length, causing the add action to hide early; change
the check to use the same total count as the rest of the code (e.g., replace
manualCount with totalCount = waypoints.length or create a shared helper like
getWaypointCount that returns the full waypoint count) and update both this
check and addWaypoint/add-waypoint render guard to call that same helper so
station-preview legs are consistently counted against MAX_WAYPOINTS.
🪄 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: b0e81788-78f5-43e2-bf80-8cbd5d63c1b1
📒 Files selected for processing (2)
src/components/home-client.tsxsrc/components/search/search-panel.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/home-client.tsx
Comment on lines
+141
to
+145
| (o: Location, d: Location, wps: WaypointEntry[], options?: { isStationLeg?: boolean }) => { | ||
| const wpCoords = wps | ||
| .filter((wp) => wp.location != null) | ||
| .map((wp) => wp.location!.coordinates); | ||
| onRoute(o.coordinates, d.coordinates, wpCoords.length > 0 ? wpCoords : undefined); | ||
| onRoute(o.coordinates, d.coordinates, wpCoords.length > 0 ? wpCoords : undefined, options); |
There was a problem hiding this comment.
Exclude isStationLeg entries from base-route recalcs.
waypoints now mixes real stops and preview-only station legs, but calculateRoute() always forwards every resolved waypoint. After a station click, any later normal recalculation will send that preview leg through onRoute as part of the base route and re-fetch corridors on the wrong path.
Suggested fix
(o: Location, d: Location, wps: WaypointEntry[], options?: { isStationLeg?: boolean }) => {
const wpCoords = wps
- .filter((wp) => wp.location != null)
+ .filter((wp) => wp.location != null && (options?.isStationLeg || !wp.isStationLeg))
.map((wp) => wp.location!.coordinates);
onRoute(o.coordinates, d.coordinates, wpCoords.length > 0 ? wpCoords : undefined, options);
},Also applies to: 196-198
🤖 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 141 - 145, When building
the waypoint coordinates passed to onRoute (the wpCoords used by
calculateRoute/route callback), filter out preview-only station legs by
excluding WaypointEntry items with isStationLeg truthy before mapping to
.location.coordinates; i.e., change the wpCoords formation in the anonymous
route callback (and the analogous code around lines 196-198) to .filter(wp =>
wp.location != null && !wp.isStationLeg).map(wp => wp.location!.coordinates) so
onRoute receives only real stops for base-route recalcs.
…eg click Two fixes: 1. Detour baseline: replace linear-interpolation baseline (routeDuration * fraction) with a parallel Valhalla exit→rejoin route. The old approach assumed uniform speed along the entire route, which was wildly wrong on mixed highway/town routes (showed +39min instead of +12min for Erla). 2. Station-leg click: clicking a station now recalculates the route through it (display only) without re-fetching corridor stations. Base routes in routeState still control corridor fetching. MapView gets a new displayRoutes prop used only for route-layer rendering.
…oint removal When selectedStationId becomes null (popup close, click elsewhere), an effect in search-panel removes the station-leg waypoint and calls onClearStationLeg, which clears stationLegRoutes in home-client. The display falls back to base routes without re-fetching corridors. Removing a station-leg waypoint via the X button now skips the wpRouteVersion bump, so it only clears the preview route instead of triggering a full route+corridor recalculation.
Station-leg and normal route requests now use separate abort refs (stationLegAbortRef vs routeAbortRef). Clearing the station-leg preview aborts the in-flight request so a late response can't resurrect stationLegRoutes after the user already deselected. Normal route requests also abort any pending station-leg preview.
Wrap processStation in try/catch so a single Valhalla timeout returns detourMin: -1 instead of failing the whole batch with HTTP 500. Add setIsRouteLoading(false) to handleSelectStation(null) and handleClearStationLeg so aborting a station-leg preview doesn't leave the loading spinner stuck.
Guard setIsRouteLoading(false) in handleSelectStation(null) and handleClearStationLeg behind !routeAbortRef.current so that closing a station popup during a swap/recalc doesn't hide the spinner.
routeAbortRef.current was never cleared after a normal route finished, staying truthy forever. The station-leg clear handlers checked it to decide whether to reset the spinner, but the stale ref made them skip the reset. Now the finally block nulls the ref when its controller is still current, giving clear handlers an accurate in-flight signal.
handleSelectRoute cleared stationLegRoutes but did not abort stationLegAbortRef, so a pending station-leg request could still resolve and overwrite the newly selected base route.
GeiserX
force-pushed
the
fix/valhalla-baseline-detour
branch
from
9fae53e to
8bd6d5e
Compare
This was referenced Apr 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
routeDuration * fraction) with a parallel Valhallaexit → rejoinroute call. The old approach assumed uniform speed along the entire route, producing wildly inaccurate detour estimates on mixed highway/town routes (e.g. +39min displayed instead of the real +12min for Erla on Sangüesa→Murcia).stationLegRoutesstate in home-client stores the display-only route, while the baserouteStatestill controls corridor fetching. MapView receives a newdisplayRoutesprop used exclusively for route-layer rendering.Test plan
Summary by CodeRabbit
New Features
Bug Fixes