Skip to content

feat: add smooth fade-in transition for route polylines#442

Open
Siddhantbg wants to merge 3 commits intoOneBusAway:developfrom
Siddhantbg:feature/polyline-animation
Open

feat: add smooth fade-in transition for route polylines#442
Siddhantbg wants to merge 3 commits intoOneBusAway:developfrom
Siddhantbg:feature/polyline-animation

Conversation

@Siddhantbg
Copy link
Copy Markdown

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

  • Implemented opacity-based animation for route polylines
  • Used requestAnimationFrame for smooth and lightweight transitions
  • Removed the TODO comment in RouteMap.svelte

Implementation Details

  • Polylines are initially rendered with zero opacity
  • Opacity is progressively increased to the target value
  • The animation is designed to avoid performance issues or UI lag

Testing

  • Verified that route polylines animate smoothly when selected
  • Confirmed no visual jank during rendering
  • Application runs successfully after changes

Notes

  • Implementation is currently applied to the OpenStreetMap provider
  • Can be extended to other providers if needed

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Copy Markdown
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. Two existing tests are broken — The serverCache.test.js suite has 2 failing tests after this change:

    • background fetch resolves after timeout and populates cache
    • cooldown expiry allows retry after ERROR_RETRY_DELAY

    The withRetry wrapper changes the call count expectations in these tests. All tests must pass before merging — please run npm run test and fix the failures.

  2. 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)

  1. Formatting failuresrc/lib/serverCache.js fails Prettier checks. Run npm run format before pushing.

  2. polyline._map is a Leaflet internalOpenStreetMapProvider.svelte.js:471 — Accessing polyline._map relies 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 public polyline.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)

  1. animate defaults to true for all polylinesOpenStreetMapProvider.svelte.js:502 — Every createPolyline call will now animate by default, including walking path polylines in trip planning, which may feel odd. Consider defaulting to false and opting in where you want animation (route selection), or verify that all current callers look right with animation.

  2. No tests for new utility functions — The withRetry, mapWithConcurrency, isRateLimitError, and sleep functions in serverCache.js have 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 animatePolylineOpacity method is well-structured with proper guards (null check, SSR check, detached polyline check)
  • Good use of requestAnimationFrame for smooth animation
  • The withRetry and mapWithConcurrency utilities are well-typed with JSDoc generics
  • Exponential backoff with jitter in withRetry is the right approach for rate limit handling

Recommended Action

  1. Split into two separate PRs: polyline animation and server cache changes
  2. Fix the 2 failing tests in serverCache.test.js
  3. Run npm run format to fix the Prettier failure
  4. Consider animate default and _map internal API concerns

Verdict: Request Changes

@Siddhantbg
Copy link
Copy Markdown
Author

Thanks for the detailed feedback! This is really helpful.

I’ll work on the tasks:

  • Split the changes into separate PRs
  • Fix the failing tests in serverCache.test.js
  • Run formatting to resolve Prettier issues
  • Review the polyline map access and animation defaults

I’ll push updates shortly.

@Siddhantbg
Copy link
Copy Markdown
Author

@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:

  • Scoped the PR to only the polyline animation changes
  • Reverted server cache changes on this branch
  • Ensured tests and formatting pass

I’ll make sure to keep changes isolated to a single concern per branch going forward.
Please let me know if any further changes are needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants