Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(PredictionsChannel): filter out past predictions #2229

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

thecristen
Copy link
Collaborator

Summary of changes

Asana Ticket: Fix: Extraneous <1 minute predictions appearing on Stop Pages

Some facts:

  • because the backend endpoint that gives the stop page its schedules is cached, the frontend component refreshes every 15s, filtering out the past schedules.
    • these get merged with the predictions. but plenty of predictions have no corresponding schedule.
    • and apparently there's no frontend mechanism to filter out stale predictions
  • in the backend, past predictions get culled out of our in-memory storage every 5 minutes (gotta keep the ETS table size low)
  • the streaming connection we have to the V3 API tells us when to add/remove/update/reset our predictions

Filtering the predictions here in DotcomWeb.PredictionsChannel, right before we publish the predictions to stop pages, seems to work ok. My guess is that this logic wasn't included previously because we thought (a) we'd get timely "remove" events from the streaming predictions or (b) something else in our logic would filter these out before making it to the UI.

# Exercise
PredictionsChannel.handle_info({:new_predictions, predictions}, socket)

[_past_prediction | expected_predictions] = predictions
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[_past_prediction | expected_predictions] = predictions
expected_predictions = [now, future]

Not a huge deal, but I'm more in favor of being super explicit about exactly what we expect, rather than having to reason about what's in the predictions array.

@@ -61,4 +62,8 @@ defmodule DotcomWeb.PredictionsChannel do
Route.subway?(prediction.route.type, prediction.route.id) &&
prediction.schedule_relationship in [:cancelled, :skipped]
end

defp in_future_seconds?(prediction) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defp in_future_seconds?(prediction) do
defp in_future?(prediction) do

Nit: I don't think the fact that it's in seconds really matters?

Copy link
Contributor

@joshlarson joshlarson left a comment

Choose a reason for hiding this comment

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

I made two (kind of nit-picky) comments that I'd be interested in your thoughts on, but IMO neither one blocks approval.

@thecristen thecristen merged commit 038db3d into main Nov 22, 2024
21 checks passed
@thecristen thecristen deleted the cbj/sub-minute-predictions branch November 22, 2024 02:00
@thecristen thecristen removed the dev-green Deploy to dev-green label Nov 22, 2024
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.

2 participants