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

fix: Fix Jellyseerr Avatar Loading Issue #2197

Merged
merged 10 commits into from
Nov 27, 2024
Merged

fix: Fix Jellyseerr Avatar Loading Issue #2197

merged 10 commits into from
Nov 27, 2024

Conversation

TyxTang
Copy link
Contributor

@TyxTang TyxTang commented Nov 11, 2024

Category

Bugfix

Overview

Jellyseerr version 2.0.1 changed rewrite avatarproxy and CachedImage
Request address changed to add /avatarproxy
This prevented the correct user avatar from being requested in the media request component.
Adapting to use older versions of Jellyseerr

Issue

Related PR

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi 👋. Thank you for making your first contribution to Homarr. Please ensure that you've completed all the points in the TODO checklist. We'll review your changes shortly.

Copy link
Collaborator

@manuel-rw manuel-rw left a comment

Choose a reason for hiding this comment

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

I dislike sending a retry request to a differnt URL.
Since this change was implemeted after a specific version, we should instead fetch the version and use the other URL if this version is above a specific threshold.

@TyxTang
Copy link
Contributor Author

TyxTang commented Nov 11, 2024

I dislike sending a retry request to a differnt URL. Since this change was implemeted after a specific version, we should instead fetch the version and use the other URL if this version is above a specific threshold.

Yes I agree with you, I'm checking the Jellyseerr API docs to implement getting the version number.

@manuel-rw
Copy link
Collaborator

manuel-rw commented Nov 11, 2024

I dislike sending a retry request to a differnt URL. Since this change was implemeted after a specific version, we should instead fetch the version and use the other URL if this version is above a specific threshold.

Yes I agree with you, I'm checking the Jellyseerr API docs to implement getting the version number.

Thank you, please tag me again for another review once you implemented my comment

@manuel-rw manuel-rw added the 🐛 Bug Something isn't working label Nov 11, 2024
@TyxTang TyxTang requested a review from manuel-rw November 11, 2024 10:02
@TyxTang
Copy link
Contributor Author

TyxTang commented Nov 11, 2024

I tried using the version comparison method, requesting /api/v1/status to get the version number,
and then comparing it to v2.0.1 was used to determine if it was the version before the change or the version after the change.

Met succes getest op zowel Jellyseerr v1.9.3 als v2.0.1

@TyxTang
Copy link
Contributor Author

TyxTang commented Nov 11, 2024

There is no way to perfectly reproduce the problem of version fetching errors, so I chose to set the default value of shouldUseV2ApiCall to false anyway and added exception detection.

I've also tested that the program works fine when Jellyseerr connectivity is fine but api/v1/status is not available.

Since I'm still in the learning stage, this is the optimal solution I came up with, if there is a better way, please correct me!

@SeDemal
Copy link
Collaborator

SeDemal commented Nov 12, 2024

This looks like a lot of pain for not much.
Can't you just produce both links and set them as src and fallbackSrc in an Image element?

@TyxTang
Copy link
Contributor Author

TyxTang commented Nov 12, 2024

This looks like a lot of pain for not much. Can't you just produce both links and set the new one as a fallback in the Image element?

It seems I don't have a good way to detect that one of the links is unavailable
Which brings us back to testing connectivity.
Maybe it could be like jumping to a preset image after not being able to get an avatar in the first place.

@SeDemal
Copy link
Collaborator

SeDemal commented Nov 12, 2024

Don't test availability and don't test connectivity then.
We can already determine that the Avatar is capable of "detecting" that the image doesn't work since it's using a fallback image.
Meaning that if we just set an Image element with one link as src, then you put the new link as fallbackSrc, the frontend will do the detection for you. Now I'm guessing you can put said Image element as a child of the Avatar, or replace the avatar and just shape the Image like the Avatar.
We don't need to know what version people are running.

Also, if people are updating Homarr, chances are they are updating other containers as well, so why are we even trying to have multiple options? Unless jellyseerr is in a beta (like we had with sonarr a while back iirc), then just update the link.

@TyxTang
Copy link
Contributor Author

TyxTang commented Nov 13, 2024

Don't test availability and don't test connectivity then. We can already determine that the Avatar is capable of "detecting" that the image doesn't work since it's using a fallback image. Meaning that if we just set an Image element with one link as src, then you put the new link as fallbackSrc, the frontend will do the detection for you. Now I'm guessing you can put said Image element as a child of the Avatar, or replace the avatar and just shape the Image like the Avatar. We don't need to know what version people are running.

Also, if people are updating Homarr, chances are they are updating other containers as well, so why are we even trying to have multiple options? Unless jellyseerr is in a beta (like we had with sonarr a while back iirc), then just update the link.

This method is great and I'm testing it to make sure it works!

@SeDemal
Copy link
Collaborator

SeDemal commented Nov 16, 2024

I just got a report on discord that Jellyseerr's new update fixed it all already, I'm guessing this was actually not done on purpose.
From what I can see, it was intended for both urls to keep working, so this implementation is still a good idea.

Copy link
Collaborator

@SeDemal SeDemal left a comment

Choose a reason for hiding this comment

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

I'll be honest, at first I was actually suggesting to only use the image element and remove the Avatar completely, but I think it's actually a good idea to keep it as a wrapper. Great job there.

The only thing left would be to remove the duplication of the functions, since that defeats the purpose of functions, but other than that it looks good to me.

Thanks for your patience and your contribution :)

@TyxTang TyxTang requested a review from SeDemal November 17, 2024 08:38
Copy link
Collaborator

@SeDemal SeDemal left a comment

Choose a reason for hiding this comment

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

lgtm

@SeDemal SeDemal merged commit b59921b into ajnart:dev Nov 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Something isn't working
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants