Lists' background images are not cached by clients #1373

New Issue

Ghost · 2023-01-25T22:14:05Z

Ghost commented

2023-01-25 22:14:05 +00:00

Description

I'm not 100% sure, but it seems like lists' background images are never cached by clients.

This is specially noticeable for large images while on mobile and using data instead of wifi. They take a long time to load every time the list is opened, or the "Namespace & Lists" page is opened.

This is reproducible on the demo site as well, I tried with this 12M image: https://upload.wikimedia.org/wikipedia/commons/d/dd/Big_%26_Small_Pumkins.JPG

After digging into it a bit, I suspect it's because here the file is being served with c.Stream which doesn't seem to return Last-Modified header in the response.

I tried changing it to c.File instead and I think it worked? I wasn't able to fully test the worst case scenario (mobile using data) because I wasn't able to build for linux-arm64 to deploy the changes to my server, but by building and deploying to my linux-x86-64 laptop and accessing it from the same machine I was able to see Last-Modified header in the responses and Firefox's Network Inspector did show Transferred: cached.

That being said, I'm not sending a PR for this since I'm not familiar with Go nor Echo, so maybe there was a reason for it being returned as a c.Stream initially?

Vikunja Frontend Version

705afa0272

Vikunja API Version

6259dd8d42

Browser and version

No response

Can you reproduce the bug on the Vikunja demo site?

Yes

Screenshots

No response

### Description I'm not 100% sure, but it seems like lists' background images are never cached by clients. This is specially noticeable for large images while on mobile and using data instead of wifi. They take a long time to load every time the list is opened, or the "Namespace & Lists" page is opened. This is reproducible on the demo site as well, I tried with this 12M image: https://upload.wikimedia.org/wikipedia/commons/d/dd/Big_%26_Small_Pumkins.JPG After digging into it a bit, I suspect it's because [here](https://kolaente.dev/vikunja/api/src/branch/main/pkg/modules/background/handler/background.go#L310) the file is being served with `c.Stream` which doesn't seem to return `Last-Modified` header in the response. I tried changing it to `c.File` instead and I think it worked? I wasn't able to fully test the worst case scenario (mobile using data) because I wasn't able to build for linux-arm64 to deploy the changes to my server, but by building and deploying to my linux-x86-64 laptop and accessing it from the same machine I was able to see `Last-Modified` header in the responses and Firefox's Network Inspector did show `Transferred: cached`. That being said, I'm not sending a PR for this since I'm not familiar with Go nor Echo, so maybe there was a reason for it being returned as a `c.Stream` initially? ### Vikunja Frontend Version 705afa0272b0fda977ff02be62060d39274e6c95 ### Vikunja API Version 6259dd8d42fd095a63623ffa3f23e2250e737b3f ### Browser and version _No response_ ### Can you reproduce the bug on the Vikunja demo site? Yes ### Screenshots _No response_

Ghost added the

kind/bug

label 2023-01-25 22:14:05 +00:00

dpschen commented

2023-01-26 09:42:57 +00:00

Wow it would be amazing if this would fix the caching.

I also realised that for example the list background endpoint (e.g. https://try.vikunja.io/api/v1/lists/3/background) does have content-type: image/jpg but isn't a jpg but instead a blob that gets transformed via window.URL.createObjectURL(new Blob([response.data])). I always thought the caching problems were related to that the response is a blob and not an image and it would be best if the browser would actually cache both: the downloaded blob and the converted image.

I do not remember why the service answers via blobs instead of the correct file in the first place. Would this change fix that?

I once tried to fix this on the frontend (if I remember correctly via caching in a service worker) but it seemed more effort than I had time at that moment.

Related, but not the same issue:
We really shouldn't push 12mb images to the client and have some kind of image proxy that helps us with the creation of images for srcset. Or do we have that already?

Wow it would be amazing if this would fix the caching. I also realised that for example the list background endpoint (e.g. https://try.vikunja.io/api/v1/lists/3/background) does have `content-type: image/jpg` but isn't a jpg but instead a blob that gets transformed via `window.URL.createObjectURL(new Blob([response.data]))`. I always thought the caching problems were related to that the response is a blob and not an image and it would be best if the browser would actually cache both: the downloaded blob and the converted image. I do not remember why the service answers via blobs instead of the correct file in the first place. Would this change fix that? I once tried to fix this on the frontend (if I remember correctly via caching in a service worker) but it seemed more effort than I had time at that moment. Related, but not the same issue: We really shouldn't push 12mb images to the client and have some kind of image proxy that helps us with the creation of images for `srcset`. Or do we have that already?

konrad commented

2023-01-26 10:44:54 +00:00

The main reason to use c.Stream instead of c.File is the latter expects a file on disk. The way the storage is abstracted in Vikunja allows expanding this away from disk and to something like S3 in the future. In that case, we won't be able to use c.File anyway.

I think the proper solution would be to simply set the headers manually instead of relying on c.File doing that.

I wasn't able to fully test the worst case scenario (mobile using data) because I wasn't able to build for linux-arm64 to deploy the changes to my server, but by building and deploying to my linux-x86-64 laptop and accessing it from the same machine I was able to see Last-Modified header in the responses and Firefox's Network Inspector did show Transferred: cached.

You could try the network speed throttle (I think that's only available in Chromium though)

I do not remember why the service answers via blobs instead of the correct file in the first place. Would this change fix that?

The fix would not change that. The background is only accessible with a JWT token and hence we need to put it in a blob after requesting it (with axios) and can't just let the browser do it with an <img src="/api/v1/lists/3/background"/>.

We really shouldn't push 12mb images to the client and have some kind of image proxy that helps us with the creation of images for srcset. Or do we have that already?

We don't! That's another problem, related to the content-type header issue (the header is always image/jpg, even if you upload a png).

Any uploaded background should be converted to jpg with a quality of maybe 90 and resized to something like 4K if bigger before saving it to disk (that should solve this 12mb problem already). When requesting it, we should allow getting resized variants from a background image like we already can for avatars.

The main reason to use `c.Stream` instead of `c.File` is the latter expects a file on disk. The way the storage is abstracted in Vikunja allows expanding this away from disk and to something like S3 in the future. In that case, we won't be able to use `c.File` anyway. I think the proper solution would be to simply set the headers manually instead of relying on `c.File` doing that. > I wasn't able to fully test the worst case scenario (mobile using data) because I wasn't able to build for linux-arm64 to deploy the changes to my server, but by building and deploying to my linux-x86-64 laptop and accessing it from the same machine I was able to see Last-Modified header in the responses and Firefox's Network Inspector did show Transferred: cached. You could try the network speed throttle (I think that's only available in Chromium though) > I do not remember why the service answers via blobs instead of the correct file in the first place. Would this change fix that? The fix would not change that. The background is only accessible with a JWT token and hence we need to put it in a blob after requesting it (with axios) and can't just let the browser do it with an `<img src="/api/v1/lists/3/background"/>`. > We really shouldn't push 12mb images to the client and have some kind of image proxy that helps us with the creation of images for srcset. Or do we have that already? We don't! That's another problem, related to the `content-type` header issue (the header is always `image/jpg`, even if you upload a png). Any uploaded background should be converted to jpg with a quality of maybe 90 and resized to something like 4K if bigger before saving it to disk (that should solve this 12mb problem already). When requesting it, we should allow getting resized variants from a background image like we already can for avatars.

👍 1

Ghost commented

2023-01-27 23:42:24 +00:00

I think the proper solution would be to simply set the headers manually instead of relying on c.File doing that.

Cool, I tried this and seems to be working fine, I'll send a PR.

> I think the proper solution would be to simply set the headers manually instead of relying on c.File doing that. Cool, I tried this and seems to be working fine, I'll send a PR.

dpschen commented

2023-02-11 12:22:03 +00:00

This can be closed, since it was fixed by vikunja/api#1376 right?

This can be closed, since it was fixed by https://kolaente.dev/vikunja/api/pulls/1376 right?

konrad closed this issue

2023-02-13 10:00:42 +00:00

konrad referenced this issue from a commit

2024-02-07 14:00:31 +00:00

chore(deps): update dependency vite to v2.7.12 (#1373)

Sign in to join this conversation.