Skip to content

Conversation

davidcornu
Copy link
Member

@davidcornu davidcornu commented Aug 8, 2025

Fixes #11263

Summary of the problem

Context

  • The transaction page uses a turbo frame to lazy load the ledger:
    <%= turbo_frame_tag [dom_id(@event), :ledger], src: event_ledger_path(@event, **params.to_unsafe_h) do %>
    <%= render partial: "filter" %>
    <%= render "application/loading_container", klass: "mt-8" %>
    <% end %>
  • The contents of the turbo frame are rendered by the .../ledger endpoint (note that these contain the filters as well):
    <%= turbo_frame_tag [dom_id(@event), :ledger] do %>
    <%= render partial: "filter" %>
  • The filters are a mix of links
    <%= link_to(upsert_query_params(type:), class: "flex-auto menu__action #{"menu__action--active" if @type == type}", data: { turbo_prefetch: "false" }) do %>
    <%= type.humanize.gsub("Ach", "ACH").gsub("Paypal", "PayPal").gsub("Hcb", "HCB") %>
    <% end %>
    and forms
    <%= form_with(model: nil, local: true, method: :get, class: "flex-auto p1") do |form| %>
    <p class="bold mb0 mt0">Transactions after...</p>
    <%= form.date_field :start, class: "border mb1", value: @start_date %>
    <p class="bold mb0 mt0">Transactions before...</p>
    <%= form.date_field :end, class: "border mb1", value: @end_date %>
  • When the user clicks on the filter icon, these filter options are rendered inside of a menu
    <div data-controller="menu" data-menu-append-to-value="turbo-frame#ledger" data-menu-placement-value="bottom-start">

Where this went wrong

  1. Our menu component supports a data-menu-append-to-value attribute which determines where the DOM for the menu is rendered. This was configured in this case but pointed to an ID that did not exist (turbo-frame#ledger), which meant the menu was being appended outside of the <turbo-frame> element.
  2. This meant that when one of the filter forms was submitted, turbo treated this as a full page visit to the .../ledger endpoint, and when the response didn't include the full page structure it bailed and issued a full page reload of that URL (likely because of the behaviour described in https://turbo.hotwired.dev/handbook/drive#reloading-when-assets-change)

Describe your changes

  • Fixes the data-menu-append-to-value attribute.
  • Scopes the params passed to the ledger turbo frame to just request.query_parameters instead of all of params as that led to action and controller being set which caused havoc with all the link_to(upsert_query_params... calls we had
  • Uses explicit paths in all link_tos

@davidcornu davidcornu requested review from a team as code owners August 8, 2025 19:44
@@ -141,7 +141,7 @@

<%= render "pinned_transactions" %>

<%= turbo_frame_tag [dom_id(@event), :ledger], src: event_ledger_path(@event, **params.to_unsafe_h) do %>
<%= turbo_frame_tag [dom_id(@event), :ledger], src: event_ledger_path(@event, request.query_parameters) do %>
Copy link
Member Author

Choose a reason for hiding this comment

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

This meant we were rendering the ledger endpoint with action and controller parameters which was really messing with the link_to calls in app/views/events/_filter.html.erb as we would get the transactions URL on initial load but the ledger URL on subsequent ones.

@davidcornu davidcornu force-pushed the david/push-yqpunnwtlvnm branch from c5014a1 to 32a0ec7 Compare August 8, 2025 20:47
Copy link
Member

@sampoder sampoder left a comment

Choose a reason for hiding this comment

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

thank you!

@davidcornu davidcornu added this pull request to the merge queue Aug 11, 2025
Merged via the queue into main with commit 49f7011 Aug 11, 2025
13 checks passed
@davidcornu davidcornu deleted the david/push-yqpunnwtlvnm branch August 11, 2025 14:09
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.

[Bug] Stylesheets don't load on slow-loading pages
2 participants