feat: Smooth route fit, draw animation, and vehicle movement on the map#518
feat: Smooth route fit, draw animation, and vehicle movement on the map#518Ahmedhossamdev wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR introduces route-aware vehicle marker animation in a new ChangesRoute Animation and Map Camera Improvements
i18n Locale Error Handling Fix
Sequence Diagram(s)sequenceDiagram
participant Component as SearchPane / RouteMap
participant Provider as GoogleMapProvider / OpenStreetMapProvider
participant animateMarker as animateMarker.js
rect rgba(70, 130, 180, 0.5)
note over Component,Provider: Route load & camera fit
Component->>Provider: createPolyline (awaited)
Component->>Provider: fitToPolylines()
Provider-->>Component: Promise resolves (or false)
alt fit failed
Component->>Provider: flyTo(midLat, midLng, zoom)
end
end
rect rgba(60, 179, 113, 0.5)
note over Provider,animateMarker: Vehicle position update
Provider->>Provider: _getRoutePaths()
Provider->>animateMarker: animateMarkerTo(marker, from, to, setPosition, {routePaths})
animateMarker->>animateMarker: buildRoutePath(routePaths, from, to)
animateMarker->>animateMarker: runAnimation via rAF (cubic ease-out)
animateMarker-->>Provider: marker reaches destination
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Coverage is 80.732% — feature/map-route-and-vehicle-animations into develop. No base build found for |
…prove route fitting logic, and implement smooth marker movement for vehicle updates
… tests for path building
Ahmedhossamdev
force-pushed
the
feature/map-route-and-vehicle-animations
branch
from
7a3d49b to
1c16e5a
Compare
![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 (3)
src/lib/Provider/OpenStreetMapProvider.svelte.js (2)
591-626: 💤 Low valueStale callback risk if polylines are cleared during the draw animation.
The
setTimeoutat line 617 referencespathandpolyline.arrowDecoratorwhich may have been removed ifclearAllPolylines()is called during the animation. While not critical (worst case is a no-op or brief console warning), consider tracking active timeouts for cleanup.💡 Optional defensive guard
setTimeout(() => { + // Bail if the polyline was removed mid-animation. + if (!this.polylines.includes(polyline)) return; // Clear the inline styles so the original stroke (e.g. the dashed // pattern used for walking legs) is restored once drawing is done. path.style.transition = '';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/Provider/OpenStreetMapProvider.svelte.js` around lines 591 - 626, The _revealPolylinesWithDraw method creates setTimeout callbacks that reference path and polyline.arrowDecorator, but these references can become stale if clearAllPolylines() is called during the animation. Store the timeout ID created in the setTimeout call (around line 617) in a collection on the polyline object or a class-level map, then add cleanup logic to cancel pending timeouts when polylines are removed or cleared. This ensures that if clearAllPolylines() executes during an animation, any outstanding timeouts for those polylines are canceled before they can attempt to access already-removed DOM elements or decorator objects.
628-674: 💤 Low valueFallback timer may fire after polylines are cleared, causing stale state access.
If the user navigates away or triggers
clearAllPolylines()before themoveendevent or the 250ms fallback fires,reveal()will call_revealPolylinesWithDrawon an emptythis.polylinesarray. This is harmless (no-op loop) but the Promise resolves withtrueeven though nothing was revealed.Consider guarding or returning early in
reveal()ifthis.polylines.length === 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/Provider/OpenStreetMapProvider.svelte.js` around lines 628 - 674, The `reveal()` callback function inside the `fitToPolylines()` method can execute after polylines are cleared, causing it to call `_revealPolylinesWithDraw()` on an empty array and incorrectly resolve the Promise with `true`. Add an early return guard at the start of the `reveal()` function to check if `this.polylines.length === 0`, and if so, return early or resolve with `false` instead, ensuring the Promise accurately reflects whether any polylines were actually revealed.src/lib/Provider/GoogleMapProvider.svelte.js (1)
591-622: 💤 Low valueIdle listener is not removed if
fitBoundsresolves synchronously or the map is destroyed.The
addListenerOnceshould handle most cases, but if the map is destroyed beforeidlefires, the listener and Promise remain dangling. Consider adding a cleanup mechanism or documenting this limitation.However, since
addListenerOnceself-removes after firing, the practical risk is low for normal usage patterns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/Provider/GoogleMapProvider.svelte.js` around lines 591 - 622, The fitToPolylines method adds an idle event listener that may not be removed if the map is destroyed before the idle event fires, potentially leaving a dangling listener and unresolved Promise. To fix this, store the listener ID returned by addListenerOnce and add a cleanup mechanism that removes the listener if the map becomes unavailable or is destroyed before the idle event fires. This could be implemented by either checking if the map still exists before resolving or by adding a destroy/cleanup handler to the map instance that removes the listener and rejects the Promise if triggered before the idle event fires.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/search/SearchPane.svelte`:
- Around line 128-132: The polylines array is being appended to repeatedly in
handleRouteClick without being cleared first, causing stale polyline references
to accumulate across multiple route selections. Before the for loop that
iterates over polylinesData and calls mapProvider.createPolyline, clear the
polylines array by resetting it to an empty array. This ensures that each route
click rebuilds the polylines collection from scratch rather than appending to
previous values.
---
Nitpick comments:
In `@src/lib/Provider/GoogleMapProvider.svelte.js`:
- Around line 591-622: The fitToPolylines method adds an idle event listener
that may not be removed if the map is destroyed before the idle event fires,
potentially leaving a dangling listener and unresolved Promise. To fix this,
store the listener ID returned by addListenerOnce and add a cleanup mechanism
that removes the listener if the map becomes unavailable or is destroyed before
the idle event fires. This could be implemented by either checking if the map
still exists before resolving or by adding a destroy/cleanup handler to the map
instance that removes the listener and rejects the Promise if triggered before
the idle event fires.
In `@src/lib/Provider/OpenStreetMapProvider.svelte.js`:
- Around line 591-626: The _revealPolylinesWithDraw method creates setTimeout
callbacks that reference path and polyline.arrowDecorator, but these references
can become stale if clearAllPolylines() is called during the animation. Store
the timeout ID created in the setTimeout call (around line 617) in a collection
on the polyline object or a class-level map, then add cleanup logic to cancel
pending timeouts when polylines are removed or cleared. This ensures that if
clearAllPolylines() executes during an animation, any outstanding timeouts for
those polylines are canceled before they can attempt to access already-removed
DOM elements or decorator objects.
- Around line 628-674: The `reveal()` callback function inside the
`fitToPolylines()` method can execute after polylines are cleared, causing it to
call `_revealPolylinesWithDraw()` on an empty array and incorrectly resolve the
Promise with `true`. Add an early return guard at the start of the `reveal()`
function to check if `this.polylines.length === 0`, and if so, return early or
resolve with `false` instead, ensuring the Promise accurately reflects whether
any polylines were actually revealed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0dc5655-acb8-493d-977c-005f831944b4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
src/assets/styles/leaflet-map.csssrc/components/map/RouteMap.sveltesrc/components/routes/RouteModal.sveltesrc/components/routes/__tests__/RouteModal.test.jssrc/components/search/SearchPane.sveltesrc/lib/MapHelpers/animateMarker.jssrc/lib/Provider/GoogleMapProvider.svelte.jssrc/lib/Provider/OpenStreetMapProvider.svelte.jssrc/lib/i18n.jssrc/tests/lib/animateMarker.test.js
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Hi Ahmed — this is a really enjoyable PR to review. The map has needed exactly this kind of polish for a long time, and you've gone after it thoughtfully: the route now frames itself to its full extent, draws itself in, and vehicles glide along the shape instead of teleporting. The buildRoutePath projection math is genuinely nice work, and it's backed by tests that read well and cover the tricky cases (corner-following, reversal, off-route rejection, multi-shape selection). The comments throughout are unusually careful and accurate — I checked the load-bearing ones against the actual library source and they hold up.
There's one issue that has to be fixed before this can merge: the Google fitToPolylines implementation can hang the entire route view with no error surfaced. Details below.
Critical Issues (1 found)
-
GoogleMapProvider.fitToPolylines()can hang forever — no timeout fallback. [src/lib/Provider/GoogleMapProvider.svelte.js:603-625]
The returned Promise resolves only from inside the one-shotidlelistener. Google Maps does not guarantee anidleevent whenfitBoundsproduces no visible viewport change (bounds already fitted, a degenerate single-point bound that survives theisEmpty()check, or the map mid-gesture). Ifidlenever fires, the Promise never settles.Both callers
awaitthis and gate everything downstream on it:SearchPane.handleRouteClick[src/components/search/SearchPane.svelte:138] —showStopsOnRoute(line 146) andfetchAndUpdateVehicles(line 152) never run.RouteMap.loadRouteData[src/components/map/RouteMap.svelte:70] — the stop-marker loop (78-83) and vehicle polling (86) never run.
The result is a textbook silent failure: no throw (so the surrounding
try/catchnever fires), no log, no feedback — just a half-drawn route with no stop markers and no vehicles. Your own OSM sibling already guards this exact scenario with asetTimeoutfallback [OpenStreetMapProvider.svelte.js:664-666], which is what makes the omission on the Google side look like an oversight. Mirror it, with asettledguard to avoid a double-resolve:return new Promise((resolve) => { let settled = false; const finish = () => { if (settled) return; settled = true; if (this.map.getZoom() > maxZoom) this.map.setZoom(maxZoom); resolve(true); }; window.google.maps.event.addListenerOnce(this.map, 'idle', finish); setTimeout(finish, 1000); // safety net if `idle` never fires this.map.fitBounds(bounds, options.padding ?? 50); });
Important Issues (3 found)
-
Un-cancelled
setTimeoutin_revealPolylinesWithDrawfires against polylines removed mid-draw. [src/lib/Provider/OpenStreetMapProvider.svelte.js:617-624]
The per-polyline timeout that clears inline styles and adds the arrow decorator is never tracked or cancelled. Both callers start a new route load withclearAllPolylines()(which runspolyline.remove()). If a user clicks a second route within the ~1.2s draw window, the pending callback runsaddDecorator()against a polyline that was just cleared, leaving a stray arrow decorator with no backing line. No crash, but an intermittent ghost-arrow on rapid route switching that's hard to diagnose. Store the timer id on the polyline andclearTimeoutit inremovePolyline/clearAllPolylines, and bail out of the callback if!this.map.hasLayer(polyline). -
OSM
fitToPolylinesreveal closure can draw the wrong route after a fast switch. [src/lib/Provider/OpenStreetMapProvider.svelte.js:654-666]
reveal()calls_revealPolylinesWithDraw, which iteratesthis.polylines— the live array, not a snapshot. If route B loads before route A'smoveend/fallback fires, A's pendingrevealdraws B's polylines (or the now-empty set). Therevealedguard only dedupes A's own two triggers; it doesn't protect against a superseded load. Capture the polyline set (or a load token) whenfitToPolylinesstarts and no-op the reveal if it no longer matches. -
The animation runtime is essentially untested. [
src/tests/lib/animateMarker.test.js]
buildRoutePathis well covered, but the module's actual entry point —animateMarkerTo— andcancelMarkerAnimationhave no tests, and neither does the cumulative-distance interpolation (measurePath/positionAlong) that decides whether the glide looks correct. These are cheap to add with the patterns already in this repo (vi.useFakeTimers,vi.stubGlobal('requestAnimationFrame', …)). Worth covering before merge:cancelMarkerAnimation— callscancelAnimationFramewith the stored id, nulls_animationFrameId, and no-ops on a null marker (theremoveVehicleMarkerpath can hit that).animateMarkerToinstant-move fallback —from === toand no-requestAnimationFrameboth callsetPosition(to…)once and schedule no frame.- Interpolation end-state — the final frame lands exactly on
to, a midpoint on an L-shaped route sits on the geometry (not the diagonal), and a zero-length path doesn't divide by zero.
A test asserting
fitToPolylinesresolves even when no camera move occurs would also have caught the Critical issue above. The provider methods that depend on live Leaflet/Google globals (_revealPolylinesWithDraw, the bounds math) are fine to leave untested — that's consistent with the rest of the repo.
Suggestions (3 found)
- Make the Google
flyTooptions contract explicit. [src/lib/Provider/GoogleMapProvider.svelte.js:580-586] The comment correctly notes the trailingoptionsarg is "harmlessly ignored," but relying on silent arg-dropping rots if the OSM signature changes. ConsiderflyTo(lat, lng, zoom = 15, _options = {})so the shared interface is enforced rather than coincidental. - Normalize the
createPolylinenull-vs-throw contract. OSMcreatePolylinereturnsnullon decode failure; Google throws. Both are handled safely in the current call paths, but the asymmetry means a future caller can't assume a uniform contract. Worth documenting or normalizing. - The 7-line i18n comment is at the verbose end. [
src/lib/i18n.js:118-127] Accurate and the nuance is real, but the parentheticalen-US-closest-matching example is the load-bearing part; the rest could be trimmed.
Strengths
buildRoutePathand the projection helpers are sound — equirectangular scaling is appropriate over short vehicle-update spans, segment projection clamps correctly, and forward/backward vertex walking handles the same-segment case.- Animation cleanup is good defensive design:
cancelMarkerAnimationruns before each re-animation and on every marker removal/clear path on both providers — no rAF leaks. - The off-route / null-shape fallbacks degrade gracefully to straight-line interpolation; the marker always moves.
- The i18n
Promise.resolve(locale.set(...))fix is correct (verified againstsvelte-i18n@4.0.1source) and properly tested. - The OSM "hide route during the glide, draw it once the basemap settles" approach is a clever, correctly-reasoned fix for the MapLibre GL / SVG desync.
Recommended Action
- Fix the Critical Google
fitToPolylineshang (add thesetTimeout+settledguard). - Address the two OSM rapid-switch races (un-cancelled timer, stale reveal closure).
- Add the three animation-runtime tests.
- Consider the suggestions.
- Re-run the review after fixes.
Verdict: Request Changes — the route view can permanently hang on the Google provider with no error surfaced, and that path is reachable in normal use.
Fair point, I'm working on the fix. |
… safety checks, add animation tests
3 participants
Improves the map UX for route display and real-time vehicles.
Changes
fitToPolylines()on both map providers centers the full route with padding and a max-zoom cap, replacing the hardcoded zoom inRouteMapandSearchPane(arrival --> trip, and "view all routes" flows).flyToBoundsglide (avoids the MapLibre GL vs SVG desync), then "draws" itself start --> end viastroke-dashoffset.flyToaccepts{ animate: false }; the route stop list uses it so the route stays glued to the basemap.animateMarkerhelper interpolates vehicle markers between position updates (~1.2s ease-out) so they glide along the route instead of teleporting; animations are cancelled on marker removal.Summary by CodeRabbit