feat: add smooth fade-in transition for route polylines#442
feat: add smooth fade-in transition for route polylines#442Siddhantbg wants to merge 3 commits intoOneBusAway:developfrom
Conversation
|
|
aaronbrethorst
left a comment
There was a problem hiding this comment.
Hey Siddhant, the polyline fade-in animation is a nice UX touch — the requestAnimationFrame approach with SSR and detach guards shows good attention to edge cases. There are a few issues to work through before this can merge, though.
Critical Issues (2 found)
-
Two existing tests are broken — The
serverCache.test.jssuite has 2 failing tests after this change:background fetch resolves after timeout and populates cachecooldown expiry allows retry after ERROR_RETRY_DELAY
The
withRetrywrapper changes the call count expectations in these tests. All tests must pass before merging — please runnpm run testand fix the failures. -
Unrelated changes bundled together — This PR combines two completely independent features:
- Polyline fade-in animation (OSM provider, ~42 lines)
- Server cache rate limiting and concurrency control (~110 lines:
withRetry,mapWithConcurrency,isRateLimitError, concurrency-limited route fetching)
These should be separate PRs. The server cache changes are a significant behavioral change to how the app fetches data on startup — they deserve their own review, testing, and commit history. Please split this into two PRs.
Important Issues (2 found)
-
Formatting failure —
src/lib/serverCache.jsfails Prettier checks. Runnpm run formatbefore pushing. -
polyline._mapis a Leaflet internal —OpenStreetMapProvider.svelte.js:471— Accessingpolyline._maprelies on Leaflet's private API (the underscore prefix convention). If Leaflet changes this internal in a future version, the animation will silently break. Consider using the publicpolyline.getPane()or wrapping the check in a try/catch, or at minimum add a comment noting this is an internal API dependency.
Suggestions (2 found)
-
animatedefaults totruefor all polylines —OpenStreetMapProvider.svelte.js:502— EverycreatePolylinecall will now animate by default, including walking path polylines in trip planning, which may feel odd. Consider defaulting tofalseand opting in where you want animation (route selection), or verify that all current callers look right with animation. -
No tests for new utility functions — The
withRetry,mapWithConcurrency,isRateLimitError, andsleepfunctions inserverCache.jshave no dedicated tests. These are non-trivial (exponential backoff with jitter, concurrency pool) and would benefit from unit tests, especially given the existing test failures.
Strengths
- The
animatePolylineOpacitymethod is well-structured with proper guards (null check, SSR check, detached polyline check) - Good use of
requestAnimationFramefor smooth animation - The
withRetryandmapWithConcurrencyutilities are well-typed with JSDoc generics - Exponential backoff with jitter in
withRetryis the right approach for rate limit handling
Recommended Action
- Split into two separate PRs: polyline animation and server cache changes
- Fix the 2 failing tests in
serverCache.test.js - Run
npm run formatto fix the Prettier failure - Consider
animatedefault and_mapinternal API concerns
Verdict: Request Changes
|
Thanks for the detailed feedback! This is really helpful. I’ll work on the tasks:
I’ll push updates shortly. |
|
@aaronbrethorst Thanks for the detailed feedback! The issue was caused because I by mistake mixed the unrelated changes in the same branch (server cache work along with the polyline animation), which led to the failing tests and extra diff. I’ve now:
I’ll make sure to keep changes isolated to a single concern per branch going forward. |
Summary
This PR adds a smooth fade-in transition for route polylines to improve the user experience when displaying route shapes on the map.
Changes
Implementation Details
Testing
Notes