Move category dropdown menu content into a turbo frame #782

2024-05-21T04:27:08+08:00

jakubkottnauer commented

2024-05-21 04:27:08 +08:00

(Migrated from github.com)

This PR fixes (or at least improves by a large margin) the performance issues reported in #739.

Each transaction row comes with a huge amount of HTML representing the contents of the category dropdown, 99% of which will never viewed by the user. Moving the dropdown contents into a lazily loaded turbo frame makes an account page with 1k transaction about 20x smaller (2.5 MB instead of 51 MB).

Before

After

After this change there is a slight delay when you click a category badge before the dropdown contents are loaded, but it seems ok with a reasonable amount of categories. This can be further improved in the future by prefetching the dropdown contents when the badge is hovered.

This PR fixes (or at least improves by a large margin) the performance issues reported in #739. Each transaction row comes with a huge amount of HTML representing the contents of the category dropdown, 99% of which will never viewed by the user. Moving the dropdown contents into a lazily loaded turbo frame makes an account page with 1k transaction about 20x smaller (2.5 MB instead of 51 MB). Before <img width="750" alt="before" src="https://github.com/maybe-finance/maybe/assets/113784/ea7b8535-aee9-402d-be70-28705e681858"> After <img width="678" alt="after" src="https://github.com/maybe-finance/maybe/assets/113784/0502d704-cb53-4d42-81d3-4450c7e2de92"> After this change there is a slight delay when you click a category badge before the dropdown contents are loaded, but it seems ok with a _reasonable_ amount of categories. This can be further improved in the future by prefetching the dropdown contents when the badge is hovered.

zachgoll commented

2024-05-21 05:19:07 +08:00

(Migrated from github.com)

@jakubkottnauer nice idea on the lazy loaded turbo frame, I definitely agree that we should be utilizing that for this scenario.

I don't mind the slight delay, but I think we could probably add some sort of "skeleton" loader, maybe something like this?

<div class="p-6 flex items-center justify-center">
  <p class="text-sm text-gray-500 animate-pulse">Loading...</p>
</div>

Also, the category_menu_content_transaction path feels a little off to me. What about something like this?

scope module: :transactions do
        resources :categories, as: :transaction_categories do
          
          # Singular resource, resolves to `new_transaction_category_dropdown`
          resource :dropdown, only: %i[ new ], module: :categories
          
        end
end

@jakubkottnauer nice idea on the lazy loaded turbo frame, I definitely agree that we should be utilizing that for this scenario. I don't mind the slight delay, but I think we could probably add some sort of "skeleton" loader, maybe something like this? ``` <div class="p-6 flex items-center justify-center"> <p class="text-sm text-gray-500 animate-pulse">Loading...</p> </div> ``` Also, the `category_menu_content_transaction` path feels a little off to me. What about something like this? ``` scope module: :transactions do resources :categories, as: :transaction_categories do # Singular resource, resolves to `new_transaction_category_dropdown` resource :dropdown, only: %i[ new ], module: :categories end end ```

jakubkottnauer commented

2024-05-21 14:02:54 +08:00

(Migrated from github.com)

@zachgoll Thank you for the review! Though I ended up defining the route differently; your suggestion results in path /transactions/categories/:transaction_category_id/dropdown/new whereas we need something more like /transactions/:id/categories/dropdown (the dropdown needs a transaction ID in order to get the transaction's current category and mark it as selected in the UI).

Let me know if there's a nicer way to define such a route, I'm still quite new to RoR :)

@zachgoll Thank you for the review! Though I ended up defining the route differently; your suggestion results in path `/transactions/categories/:transaction_category_id/dropdown/new` whereas we need something more like `/transactions/:id/categories/dropdown` (the dropdown needs a transaction ID in order to get the transaction's current category and mark it as selected in the UI). Let me know if there's a nicer way to define such a route, I'm still quite new to RoR :)

zachgoll commented

2024-05-21 22:30:11 +08:00

(Migrated from github.com)

@jakubkottnauer let me start by saying that the following suggestions are not necessarily 100% related to this specific PR, so if you'd like to get this change in and ignore these for now, I'm fine with that. Just let me know and I'll get it merged.

But I think why we're having such a hard time with this one is because our existing routing structure for transactions and categories doesn't fit the "singular" nature of a transaction category (i.e. a transaction can have a max of 1 category). We've currently got it setup like so:

resources :transactions do
  match "search" => "transactions#search", on: :collection, via: %i[ get post ], as: :search

  collection do
    scope module: :transactions do
      resources :categories, as: :transaction_categories do
        resources :deletions, only: %i[ new create ], module: :categories
      end

      resources :rules, only: %i[ index ], as: :transaction_rules
      resources :merchants, only: %i[ index new create edit update destroy ], as: :transaction_merchants
    end
  end
end

If I'm not mistaken here, I think a better way to express all of this would be:

resources :transactions do
  match "search" => "transactions#search", on: :collection, via: %i[ get post ], as: :search

  resources :rules, only: %i[ index ], module: :transactions
  resources :merchants, only: %i[ index new create edit update destroy ], module: :transactions

  resource :category, module: :transactions do
    resource :dropdown, only: %i[ new ], module: :categories
    resource :deletion, only: %i[ new create ], module: :categories
  end
end

The routes this produces are fairly similar to what we have already, but feel a little more natural to me and expose the transaction ID that we need by default:

To implement this we'd need to rename a few folders, but IMO this would be a good improvement to what we currently have.

Would be happy to hear other ideas and critiques to this.

We'd also need a separate routing structure to independently manage these categories (i.e. in the Settings view), but I think that would be okay to have both the general editing and transaction-specific editing?

@jakubkottnauer let me start by saying that the following suggestions are not necessarily 100% related to this specific PR, so if you'd like to get this change in and ignore these for now, I'm fine with that. Just let me know and I'll get it merged. But I think why we're having such a hard time with this one is because our existing routing structure for transactions and categories doesn't fit the "singular" nature of a transaction category (i.e. a transaction can have a max of 1 category). We've currently got it setup like so: ```rb resources :transactions do match "search" => "transactions#search", on: :collection, via: %i[ get post ], as: :search collection do scope module: :transactions do resources :categories, as: :transaction_categories do resources :deletions, only: %i[ new create ], module: :categories end resources :rules, only: %i[ index ], as: :transaction_rules resources :merchants, only: %i[ index new create edit update destroy ], as: :transaction_merchants end end end ``` If I'm not mistaken here, I _think_ a better way to express all of this would be: ```rb resources :transactions do match "search" => "transactions#search", on: :collection, via: %i[ get post ], as: :search resources :rules, only: %i[ index ], module: :transactions resources :merchants, only: %i[ index new create edit update destroy ], module: :transactions resource :category, module: :transactions do resource :dropdown, only: %i[ new ], module: :categories resource :deletion, only: %i[ new create ], module: :categories end end ``` The routes this produces are fairly similar to what we have already, but feel a little more natural to me and expose the transaction ID that we need by default: ![CleanShot 2024-05-21 at 10 33 23](https://github.com/maybe-finance/maybe/assets/16676157/8f269cac-ada6-4b35-aab6-acc17e39f2ca) To implement this we'd need to rename a few folders, but IMO this would be a good improvement to what we currently have. Would be happy to hear other ideas and critiques to this. ----- We'd also need a separate routing structure to independently manage these categories (i.e. in the Settings view), but I think that would be okay to have both the general editing and transaction-specific editing?

zachgoll commented

2024-05-21 22:35:30 +08:00

(Migrated from github.com)

@josefarias no worries if you're busy with other things, but curious to hear if you'd agree with the comment above?

👀 1

zachgoll commented

2024-05-21 22:59:46 +08:00

(Migrated from github.com)

@jakubkottnauer @josefarias disregard my comment above about the singular resources. I wasn't reading things correctly before and see why we need the "collection" (to edit these outside the context of a transaction).

Wondering if we just add this above the existing route structure?

  resources :transactions do
    
    # new_transaction_category_dropdown -> /transactions/:transaction_id/category/dropdown/new
    namespace :category, module: :transactions do
      resource :dropdown, only: %i[ new ], module: :categories
    end
    
    ... existing routes
end

@jakubkottnauer @josefarias disregard my comment above about the singular resources. I wasn't reading things correctly before and see why we need the "collection" (to edit these outside the context of a transaction). Wondering if we just add this above the existing route structure? ```rb resources :transactions do # new_transaction_category_dropdown -> /transactions/:transaction_id/category/dropdown/new namespace :category, module: :transactions do resource :dropdown, only: %i[ new ], module: :categories end ... existing routes end ```

josefarias commented

2024-05-21 23:45:46 +08:00

(Migrated from github.com)

Hey @zachgoll @jakubkottnauer, great work here!

Here's how I'd suggest modeling this:

diff --git a/config/routes.rb b/config/routes.rb
index 909870a..b4ff237 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -44,6 +44,10 @@ Rails.application.routes.draw do
       scope module: :transactions do
         resources :categories, as: :transaction_categories do
           resources :deletions, only: %i[ new create ], module: :categories
+
+          collection do
+            resource :dropdown, only: :show, module: :categories, as: :transaction_categories_dropdown
+          end
         end
 
         resources :rules, only: %i[ index ], as: :transaction_rules

Could also do as: :transaction_category_dropdown if that's more natural in english, I'm not sure!

Here's the reasoning behind that:

The dropdown needs to be aware of which category is selected. We can do this via the transaction, yes. But we're not rendering "the categories for a transaction". Instead, we're rendering "all categories, but put this transaction's category at the top"
1. My suggestion is to do something like transaction_categories_dropdown_path(selection_id: @transaction.category_id)
2. That way we're using the same path for all transactions, which is easy to cache down the line if needed — but independently, it's also more semantically accurate. Again, these categories are not transaction-dependent. They're a resource by themselves. And we render the same ones every time, for every transaction. Which one goes at the top is a presentation concern and should be free to change if e.g. we ever decide not to put the selected one at the top.
Using show instead of new here because the dropdown is a resource we won't be calling create on. It's an entity that already exists. If anything, we'd call update/destroy on it — but even that doesn't make much sense in this context.
1. That said, I don't feel strongly about this. Could go with :new, no problem.

As far as rendering the actual dropdown, I'd do something like this in the controller (pseudocode, not tested):

class Transactions::Categories::DropdownsController < ApplicationController
  before_action :set_selected_category

  def show
    @categories = categories_scope.excluding(@selected_category).prepend(@selected_category).compact
  end

  private
    def set_selected_category
      if params[:selection_id]
        @selected_category = categories_scope.find(params[:selection_id])
      end
    end

    def categories_scope
      Current.family.transaction_categories.alphabetically
    end
end

Hey @zachgoll @jakubkottnauer, great work here! Here's how I'd suggest modeling this: ```diff diff --git a/config/routes.rb b/config/routes.rb index 909870a..b4ff237 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -44,6 +44,10 @@ Rails.application.routes.draw do scope module: :transactions do resources :categories, as: :transaction_categories do resources :deletions, only: %i[ new create ], module: :categories + + collection do + resource :dropdown, only: :show, module: :categories, as: :transaction_categories_dropdown + end end resources :rules, only: %i[ index ], as: :transaction_rules ``` Could also do `as: :transaction_category_dropdown` if that's more natural in english, I'm not sure! Here's the reasoning behind that: 1. The dropdown needs to be aware of which category is selected. We can do this via the transaction, yes. But we're not rendering "the categories for a transaction". Instead, we're rendering "all categories, but put this transaction's category at the top" 1. My suggestion is to do something like `transaction_categories_dropdown_path(selection_id: @transaction.category_id)` 2. That way we're using the same path for all transactions, which is easy to cache down the line if needed — but independently, it's also more semantically accurate. Again, these categories are not transaction-dependent. They're a resource by themselves. And we render the same ones every time, for every transaction. Which one goes at the top is a presentation concern and should be free to change if e.g. we ever decide not to put the selected one at the top. 2. Using `show` instead of `new` here because the dropdown is a resource we won't be calling `create` on. It's an entity that already exists. If anything, we'd call `update/destroy` on it — but even that doesn't make much sense in this context. 1. That said, I don't feel strongly about this. Could go with `:new`, no problem. As far as rendering the actual dropdown, I'd do something like this in the controller (pseudocode, not tested): ```rb class Transactions::Categories::DropdownsController < ApplicationController before_action :set_selected_category def show @categories = categories_scope.excluding(@selected_category).prepend(@selected_category).compact end private def set_selected_category if params[:selection_id] @selected_category = categories_scope.find(params[:selection_id]) end end def categories_scope Current.family.transaction_categories.alphabetically end end ```

zachgoll commented

2024-05-22 01:53:46 +08:00

(Migrated from github.com)

@josefarias appreciate the help here!

I think what you've outlined makes sense and I'd agree that show is probably a better action to use.

The helper name you came up with makes sense to me as well. As I was playing around with the routes, came up with the following, which attempts to leverage auto-generated path helper names a bit more:

resources :transactions do
  collection do
    match "search" => "transactions#search", via: %i[ get post ]

    scope module: :transactions, as: :transaction do
      resources :categories do
        resources :deletions, only: %i[ new create ], module: :categories
        resource :dropdown, only: :show, on: :collection, module: :categories
      end

      resources :rules, only: %i[ index ]
      resources :merchants, only: %i[ index new create edit update destroy ]
    end
  end
end

The only change that would be required to get tests passing again is update the set_category method in DeletionsController to use the param :category_id rather than :transaction_category_id. From a resource perspective, I think :category_id makes sense given the independent nature of categories as we've discussed above?

Any objections to this?

@josefarias appreciate the help here! I think what you've outlined makes sense and I'd agree that `show` is probably a better action to use. The helper name you came up with makes sense to me as well. As I was playing around with the routes, came up with the following, which attempts to leverage auto-generated path helper names a bit more: ```rb resources :transactions do collection do match "search" => "transactions#search", via: %i[ get post ] scope module: :transactions, as: :transaction do resources :categories do resources :deletions, only: %i[ new create ], module: :categories resource :dropdown, only: :show, on: :collection, module: :categories end resources :rules, only: %i[ index ] resources :merchants, only: %i[ index new create edit update destroy ] end end end ``` The only change that would be required to get tests passing again is update the `set_category` method in `DeletionsController` to use the param `:category_id` rather than `:transaction_category_id`. From a resource perspective, I think `:category_id` makes sense given the independent nature of categories as we've discussed above? Any objections to this?

josefarias commented

2024-05-22 01:57:26 +08:00

(Migrated from github.com)

No objections! I like that better as well

zachgoll commented

2024-05-22 02:01:30 +08:00

(Migrated from github.com)

@josefarias awesome, thanks again for the assist here!

@jakubkottnauer if you're good with the discussion and proposed changes above, I think we've got a path forward? Looks like you've got all the pieces here, just need a slight rework of the routing and the update to DeletionsController I had mentioned above.

@josefarias awesome, thanks again for the assist here! @jakubkottnauer if you're good with the discussion and proposed changes above, I think we've got a path forward? Looks like you've got all the pieces here, just need a slight rework of the routing and the update to `DeletionsController` I had mentioned above.

jakubkottnauer commented

2024-05-22 02:35:10 +08:00

(Migrated from github.com)

@josefarias @zachgoll Thank you both for the great suggestions, I think we've gotten something really nice out of it 👍 I have implemented the changes. I ended up doing

collection do
  resource :dropdown, only: :show, module: :categories, as: :category_dropdown
end
# -> /transactions/categories/dropdown

instead of

resource :dropdown, only: :show, on: :collection, module: :categories
# -> /transactions/categories/:category_id/dropdown

because the latter doesn't work when no category is selected on a transaction. I'm a bit surprised that just wrapping the resource with collection do instead of putting on: :collection directly on the resource has this kind of impact on the generated route.

@josefarias @zachgoll Thank you both for the great suggestions, I think we've gotten something really nice out of it 👍 I have implemented the changes. I ended up doing ```ruby collection do resource :dropdown, only: :show, module: :categories, as: :category_dropdown end # -> /transactions/categories/dropdown ``` instead of ```ruby resource :dropdown, only: :show, on: :collection, module: :categories # -> /transactions/categories/:category_id/dropdown ``` because the latter doesn't work when no category is selected on a transaction. I'm a bit surprised that just wrapping the resource with `collection do` instead of putting `on: :collection` directly on the resource has this kind of impact on the generated route.

zachgoll (Migrated from github.com) approved these changes 2024-05-22 04:07:34 +08:00

zachgoll (Migrated from github.com) left a comment

Looks good! Thanks for the iterations on this.

And to your question about the collection block—that's my bad, resource and resources do not accept an on option. It was just silently failing there. So I think your solution is the right one!

Looks good! Thanks for the iterations on this. And to your question about the `collection` block—that's my bad, `resource` and `resources` [do not accept](https://edgeapi.rubyonrails.org/classes/ActionDispatch/Routing/Mapper/Resources.html#method-i-resources) an `on` option. It was just silently failing there. So I think your solution is the right one!

db/schema.rb

zachgoll (Migrated from github.com) commented

2024-05-22 04:06:26 +08:00

You had updated to v16.x right? Maybe this isn't related to the PG version after all.

jakubkottnauer (Migrated from github.com) reviewed 2024-05-22 13:30:36 +08:00

db/schema.rb

jakubkottnauer (Migrated from github.com) commented

2024-05-22 13:30:36 +08:00

My bad, I haven't noticed this has snuck in again, reverted. But yes, I had upgraded to 16.3 and still get this schema change when I run bin/setup

My bad, I haven't noticed this has snuck in again, reverted. But yes, I had upgraded to 16.3 and still get this schema change when I run `bin/setup`

zachgoll (Migrated from github.com) reviewed 2024-05-22 18:32:06 +08:00

db/schema.rb

zachgoll (Migrated from github.com) commented

2024-05-22 18:32:06 +08:00

Interesting, I'll have to look into this again and see if there's a way to avoid in the future.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#782