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

feat: Cache Cover Art Calls #3015

Merged
merged 2 commits into from
Nov 19, 2024
Merged

feat: Cache Cover Art Calls #3015

merged 2 commits into from
Nov 19, 2024

Conversation

anshg1214
Copy link
Member

This PR Implements a client-side caching system for cover art using IndexedDB (with LocalStorage fallback) to improve performance and reduce server load. The system includes automatic cache expiration and cleanup mechanisms.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

I deployed to test.LB for in-situ testing, and it doesn't seem to be working for me.
I can see the stores in IndexedDB but it does not seem to store anything, the stores are empty, and going back and forth on two pages seems to call the CAA every time for all albums.

frontend/js/src/utils/coverArtCache.ts Show resolved Hide resolved
@anshg1214
Copy link
Member Author

I can see the stores in IndexedDB but it does not seem to store anything, the stores are empty

I deployed the PR back to test, and I believe the PR is working fine. Did you try refreshing the DB again?
image

and going back and forth on two pages seems to call the CAA every time for all albums.

That's strange. I checked navigating through through the pages where release cards are being rendered, things are working fine. Can you share the network logs?

@MonkeyDo
Copy link
Member

MonkeyDo commented Nov 7, 2024

Yes I had refreshed the DB, but still nothing.

This time around testing, I do see two entries (for the same cover?) in there, but the other covers are still being loaded every time.
HAR logs I will send privately as they contain cookies.

The duplicate entry with true/false at the end that I have in IndexedDB, in case it is relevant:
image

@MonkeyDo
Copy link
Member

MonkeyDo commented Nov 7, 2024

OK, so I see that the issue might be between my chair and my keyboard :)

I was looking at my dashboard when testing, I didn't realize this was going to apply only to the release card.
Re-reading your comment made me realize that.

I tested again, and I'm still not sure it's working as expected.
The SVG at the top of the artist page that loads a cover art collage makes it hard to be sure, but it still seems to hit the CAA endpoints. I tested with https://test.listenbrainz.org/artist/f58384a4-2ad2-4f24-89c5-c7b74ae1cce7/ which has many albums:

  • scrolled down and expanded to see live albums (won't be in the collage for sure)
  • scroll horizontally to load all live albums covers
  • refresh the page, wait for collage to load, purge network logs
  • scroll down and expand until I am on the live sections again
  • purge the network logs again
  • scroll the live section > 144 requests to the CAA

For context, checking the indexedDB after all that, I see 13 entries only
I am assuming I should I be seeing no requests to the CAA and more entries in indexedDB.

@@ -756,13 +757,23 @@ const getAlbumArtFromReleaseGroupMBID = async (
optionalSize?: CAAThumbnailSizes
): Promise<string | undefined> => {
try {
const cacheKey = `rag:${releaseGroupMBID}-${optionalSize}`;
Copy link
Member

Choose a reason for hiding this comment

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

rag? Release Art Group?

frontend/js/src/utils/utils.tsx Show resolved Hide resolved
@anshg1214
Copy link
Member Author

For context, checking the indexedDB after all that, I see 13 entries only

When we're displaying the release card, if the props contain caaID and caaReleaseMBID, then the calls are directly made to archive.org using this function. In this case, we don't require caching anything.

If we don't contain this information we're caching to reduce the 2 calls to coverartarchive.org, and saving the final image URL in the cache.

scroll the live section > 144 requests to the CAA

So, when you're testing on the artist page .These requests are to get the image from the URL. None of those requests are going to "fetch" the image URL from CAA.

I am assuming I should I be seeing no requests to the CAA and more entries in indexedDB.

If you want to further optimise the process, we'll need to cache the images themselves in the cache.

@MonkeyDo
Copy link
Member

OK, now I'm on your page ! Thanks for the further explanations.
In that case I think this is working as intended 🤖

@MonkeyDo MonkeyDo merged commit 63ae751 into master Nov 19, 2024
4 checks passed
@MonkeyDo MonkeyDo deleted the ansh/cache-caa-calls branch November 19, 2024 13:36
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.

2 participants