performance: Move API calls out of render and into subway_status - #3408
performance: Move API calls out of render and into subway_status#3408jlucytan wants to merge 15 commits into
Conversation
joshlarson
left a comment
joshlarson
left a comment
There was a problem hiding this comment.
I like what I see so far, and having these calls not run per-component-load is going to be a huge win!
However, I think something's gone a bit wrong with combining subheadings. Compare dev-green on the left to this branch on the right.
It looks like status on the alerts page is combining subheadings from different rows even when the rows themselves aren't being combined, but on that page, each row should only show the affected stops or endpoints from the alert associated with that row.
|
|
||
| defp decorations(%{status: status, alerts: alerts, route_ids: route_ids}) | ||
| defp decorations(%{status: status, subheading_data: {:endpoint_stops, endpoints}}) | ||
| when status in [:service_change, :shuttle, :single_tracking, :suspension] do |
There was a problem hiding this comment.
Suggestion (non-blocking): I think we can (and should) remove the when status in check here - now that you've moved the "which statuses get endpoints?" check into the backend, the frontend can just be like "endpoints? okay I'll draw them like this"
| prefix={@row.status_entry.prefix} | ||
| plural={@row.status_entry.plural} | ||
| future={@row.status_entry.future} | ||
| subheading_data={@subheading_data} |
There was a problem hiding this comment.
Suggestion (non-blocking): If you add subheading_data: nil to the "normal status" clause of rows_for_status_entry/3, then every status_entry map at this point in the code will have a subheading_data field defined, and then you can replace this with 👇, similar to all the other fields here, and get rid of the assigns-fiddling you're doing above.
<.status_row_heading
# ...
subheading_data={@row.status_entry.subheading_data}
/>
| # Override the pseudo-combined subheading_data with the specific data for this alert | ||
| subheading_data = | ||
| subheading_data( | ||
| status: alert.effect, | ||
| alerts: [alert], | ||
| route_ids: [assigns.row.route_info.route_id] | ||
| ) |
There was a problem hiding this comment.
This is only used on the alerts page, where we currently can take the performance hit (well, it's not a regression at any rate). Not ideal, would be nice to not have to call the API again (see also similar calls in planned_disruptions.ex and schedule_finder_live.ex) but it's the quickest fix without having to do wider structural changes.
2 participants
Scope
Asana Ticket: 📈 🛠️ [Subway Status] Move API calls / stop-loading out of component-rendering
Implementation
Moves
affected_stopsandendpoint_stopsout ofstatus_row_headingand intosubway_statusso there are fewer API calls at render time on the home page, and the data is cached.The alerts page and schedule finder still make calls to populate the subheading for the Planned Work section, however.
Although commuter_rail_upcoming_changes also utilizes the
status_row_headingcomponent, I don't believe it currently shows any subheadings, so no data is added there.Screenshots
Should have no visual changes.
Old flamegraph:
New flamegraph:
How to test
Homepage, alerts page, schedule finder alert banners, commuter rail upcoming changes appearances should not change between this branch and prod.