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

[BUU] Optimisation: long select lists repeated #12482

Open
dacook opened this issue May 15, 2024 · 11 comments · May be fixed by #12524
Open

[BUU] Optimisation: long select lists repeated #12482

dacook opened this issue May 15, 2024 · 11 comments · May be fixed by #12524
Assignees

Comments

@dacook
Copy link
Member

dacook commented May 15, 2024

What we should change and why (this is tech debt)

Since adding dropdowns to the bulk edit screen (/admin/products with feature toggle admin_style_v3 enabled), I've noticed the contents of the select lists are repeated for every row.

It's not too bad for short lists. But a super-admin user has access to all suppliers on the system, which can be a very long list.

Screenshot 2024-05-15 at 5 19 01 pm

Proposed solution

Each type of dropdown shares the same list of options (apart from the selected attr), so we could probably load it once, then initialise the dropdowns with JS. I think Tomselect already has options for using a data source like that.

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented May 16, 2024

I think the consequence can be this behavior (easier to reproduce as super admin), if the user clicks several dropdowns within a short time:
https://www.loom.com/share/94708479cbb74e9c87428294459d7760?sid=a6e3b4b0-d1d1-44f8-b5ef-8d7e75535902

@cyrillefr
Copy link
Contributor

Hello @dacook , @filipefurtad0 ,

May I work on this issue ?

@RachL
Copy link
Contributor

RachL commented May 17, 2024

Hi @cyrillefr yes feel free to pick, I will assign you: but it might be a tough one! Don't hesitate to ping David or Filipe again if you get stuck :)

@cyrillefr
Copy link
Contributor

Hello @dacook ,

From what I have read from the source code, data that populate the selects come from the Rails controller.

It is populated for each row in app/views/admin products_v3/_product_row.html.haml via
the SearchableDropdownComponent component (that uses tom-select).

The load once function you mention from the tom-select lib is for remote data fetching.

So I guess your idea would be to on an event(click) to populate the select with the load once.
It could avoid populating every lists, the majority of them the user would not work on.

But since, one cannot use the load once here, there would be the possibility to use some other method of the api, like the addItem, and populate the select(on a click event), while storing the data in a JS Array somewhere.
At the time I write this, I do not know if I can use hotwired/stimulus, I need to check :)

Also, when I talk about load once not possible, it is because I did not see some fetch/ajax in the source code and since you want to depart from Angular, my guess is that you would not want that. (I might be wrong though).

  • Should I go with the load once + fetch + add rails controller to return back json data in the Ajax way.
  • Or some other solution to use the data that is already present at the presentation level
  • Or let me play/experiment and chose :)

NB you mentioned the supplier lists, should the switch be applied to every tom-select list ?

@cyrillefr
Copy link
Contributor

That would imply a new component that respond to click event or to pass an option to the existing one (if possible).

@chahmedejaz
Copy link
Collaborator

chahmedejaz commented May 23, 2024

~~Hi @cyrillefr - Really sorry for getting back on this late.

I believe creating a new component for the tom-select specifically for fetching options from our server would be better. This could be more manageable I guess. You may refer to the below, I think we would want something like this:
https://tom-select.js.org/examples/remote/

The load once function you mention from the tom-select lib is for remote data fetching.

Yes, exactly but it caches the searched results as well. Not exactly caching, it just appends the options in the DOM and then looks at them before making the API call. I think it's an optimized problem like this.

NB you mentioned the supplier lists, should the switch be applied to every tom-select list ?

I think, applying this to every tom-select might be an overkill for now. However, a new component would allow us to reuse that specific tom-select for any place where we might face a similar issue.

@dacook - What do you think about it?~~

@chahmedejaz
Copy link
Collaborator

Really sorry @cyrillefr I missed the use case above 😕
That way we won't be showing options on the UI, we would have to search for it

I guess then we need to use stimulus here. We could create a generic stimulus controller which would fetch all the options from the server by the specified path and then add those options in the specified tom-select's options dynamically.
Now we need to design this controller and see how we could maintain which path to fetch the options from and how we could instruct the controller to update the specific tom-select's options.
Behind the scenes, tom-select uses the html select element, so I believe we could manipulate its options by JS.
For now we need to make it for this select field option.

Please let me know if you have any questions. Thanks.

@cyrillefr
Copy link
Contributor

Hello @chahmedejaz ,

Thanks for the comments.

The important point in my opinion here would be to populate options upon some event (click), so that only the selects that are going to be used are populated. Except for the default I guess.

Otherwise, populating every selects the way it is done currently or populating them by JS would be the same( except browser would work a bit more and server a bit less).
From what I understand, populating each and everyone select is the main issue here.

Do you see things that way too ?

@chahmedejaz
Copy link
Collaborator

The important point in my opinion here would be to populate options upon some event (click), so that only the selects that are going to be used are populated

@cyrillefr - Yes, this would indeed be a good solution in terms of having a clean DOM, however, this would cause some network delay on the UI before rendering the options. This might not be a good user experience to wait for the options upon clicking the dropdown.
I've some rough idea about how we might do that without having much delay, however I think that might add an extra layer of complexity on the Stimulus side.

From what I understand, populating each and everyone select is the main issue here.

I believe the main issue here is that the server is responding with a lot of duplicated options in the html. This is not an optimised solution in terms of response size.
By delegating the options responsibility to the JS would greatly reduce the html response size. Moreover, this may reduce the network latency of the products page response as well.

Makes sense?

@dacook
Copy link
Member Author

dacook commented May 25, 2024

Hi both, thanks for discussing this. I'm sorry, I think I forgot to respond at the start of the week.

The question is, what's the problem we're trying to solve? I have to admit I haven't measured any performance bottlenecks so I don't have an answer sorry. I raised it because I thought it was likely to cause problems, and it may have (see Filipe's post), but I haven't investigated yet. So I hadn't prioritised the task.
So if you're willing, it would be good to try and replicate Filipe's issue and see how we can fix that.

But now I'm thinking about it...

@dacook
Copy link
Member Author

dacook commented May 25, 2024

I guess there's a few levels we can optimise at, and I'm not sure what's practical. I think of at at a few levels:

  1. Rails takes resources to load the data model for each item and build the list. So we could cache that list and re-use it.
  2. Unnecessary HTML sent over the network. We could load it once over the network,
  • either embedded in the page,
  • or as a separate request. If it has cacheable header, we don't have to worry about caching it further (browser takes care of it). We could add a new controller action to generate it.
  1. The browser takes time to render the options in the DOM, many of which are never used. Can we set it up to render only when the dropdown is activated (based on preloaded data)? It could load from the cached data as per number 2.

Feel free to investigate further and try out what you think would be best.

@cyrillefr cyrillefr linked a pull request May 27, 2024 that will close this issue
4 tasks
@dacook dacook changed the title [BUU] Performance: long select lists repeated [BUU] Optimisation: long select lists repeated Jun 4, 2024
@RachL RachL removed this from the [BUU2] Product List uplift milestone Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress ⚙
Development

Successfully merging a pull request may close this issue.

5 participants