harvard-lil / perma

Indelible links
424 stars 71 forks source link

Improve folder dropdown #3645

Closed jcushman closed 3 weeks ago

jcushman commented 3 weeks ago

I started pulling a thread here, and I'm not sure if I pulled it the right amount before I stopped. So here's the story ...

After speeding up the initial page rendering on the dashboard, the next slow steps are loading the organizations and, if it's a big folder, the links:

image

which probably just means in general that the API is slow. But, I was curious about why the organizations were slow in particular, and realized that that query is almost entirely redundant of the top_level_folders data we provide in the initial page load and we may as well not do it. We first render /api/v1/user into the template, so send down everything as current_user.top_level_folders, and then we fetch /api/v1/organizations?limit=300 to get the first 300 folders over again to render the folder dropdown and get the default_to_private setting for top level folders, and that second fetch takes a full second to run. Shouldn't we just not?

So, I added registrar, registrar_name, and default_to_private to the /api/v1/user top_level_folders response:

image

And with that extra data the page can now entirely skip that /api/v1/organizations fetch. This isn't much of a performance improvement, because page load wasn't waiting on that API call to return. But it's a correctness improvement for anyone with more than 300 orgs (no one?) -- before we would have shown orgs after the first 300 as default_to_private=false regardless of their real setting, and we wouldn't have put more than 300 into the dropdown.

We don't skip the sponsored folders fetch, because those aren't top-level folders, so that one sticks around.

So, mostly-useless performance improvement for me. But in the course of adapting the folder dropdown to use top_level_folders, I found and fixed some other issues:

I think my design question is, are we happy jamming registrar, registrar_name, and default_to_private into /api/v1/user and calling it a day? The most correct thing would probably be to rethink the API a bit -- speed up the organizations endpoint, add a sponsorships endpoint, stop using the user.top_level_folders key. But I'd like to stop pulling the string for a minute so maybe this is fine.

Edit: this means a practical thing is, if you wish I hadn't messed with the API, I can pull out the optimization side of this and just keep the bug fixing side. That's a bit of a hassle but would be a reasonable ask.

rebeccacremona commented 3 weeks ago

But it's a correctness improvement for anyone with more than 300 orgs (no one?) -- before we would have shown orgs after the first 300 as default_to_private=false regardless of their real setting, and we wouldn't have put more than 300 into the dropdown.

See https://github.com/harvard-lil/perma/issues/3345

jcushman commented 3 weeks ago

An interesting side effect of the correctness improvement is the dropdown rendering is now noticeably slow (maybe a second) for admins -- I thought I had broken something, but it's just that it has 10x as many entries.

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 69.65%. Comparing base (1d33584) to head (c94aab1). Report is 12 commits behind head on develop.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #3645 +/- ## ======================================== Coverage 69.65% 69.65% ======================================== Files 54 54 Lines 7339 7339 ======================================== Hits 5112 5112 Misses 2227 2227 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sentry-io[bot] commented 3 weeks ago

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

Did you find this useful? React with a 👍 or 👎