jellyfin / jellyfin-vue

A modern web client for Jellyfin based on Vue
https://jellyfin.org
GNU General Public License v3.0
1.29k stars 229 forks source link

Invalid server config causes hang #2278

Closed pmdevita closed 6 months ago

pmdevita commented 7 months ago

Description of the bug

Jellyfin Vue stores configuration about the server you are connected to in a JSON string in Local Storage and on page load, the app tries to establish a websocket connection to it. If the connection fails, the app will retry forever to connect to the server.

Steps to reproduce

You can reproduce this by going to Local Storage in your browser's developer tools and editing the JSON string attached to the auth key. Setting auth.servers[0].PublicAddress to any URL that does not have a Jellyfin instance behind it will cause the app to hang on refresh.

Expected behavior

Retrying should probably be limited, and after a certain number of fails, the user should be sent to the menu to add a server again.

Logs

No response

Screenshots

No response

Platform

macOS

Browser

Firefox

Jellyfin server version

10.8.13

Additional context

No response

ferferga commented 7 months ago

This is no longer happening as of latest master. It was fixed a few days ago:

https://github.com/jellyfin/jellyfin-vue/blob/master/frontend/src/store/index.ts#L48

pmdevita commented 7 months ago

I tested this again on the live demo site at https://jf-vue.pages.dev/ running 19abb4dfa7e021036ca8879bac186cc4845f2632 and I'm still seeing the same result, I don't think this is fixed.

ferferga commented 7 months ago

@pmdevita After a 2nd re-read, you're right (in fact, this bug is something that I'm aware of but needs some huge work to get solved properly), my bad. Given you were mentioning the WebSocket connection (which in Firefox, given it doesn't support the Network Information API, was the only source of truth for the connection to the server), I thought you were talking about https://github.com/jellyfin/jellyfin-vue/issues/2269, which was fixed by https://github.com/jellyfin/jellyfin-vue/pull/2271, making a Ping to the server as an additional safe guard.

I'm reopening this.

ferferga commented 7 months ago

I'm still hesitant though about the real scope of the issue: I agree that if the server is unreachable you will be greeted by an endless loading screen, but I'm not sure we should handle other cases like manually modifying the localStorage.

The consistency of the data stored in localStorage data should be guaranteed as long as it's the client who access it. We can't, by any means, expect that the client acts fine if it was tampered by the user. In fact, doing that might pose a huge security risk.

pmdevita commented 7 months ago

I originally encountered this issue when I changed the domain name of my Jellyfin server, I just figured editing your local storage would be an easier strategy to reproduce the issue. Probably should have clarified that a bit more, sorry. When I did rename my Jellyfin server's domain, the Jellyfin app just brought me to the server config/login screen again so that's my rational for doing the same here.

ferferga commented 6 months ago

@pmdevita Can you confirm if https://github.com/jellyfin/jellyfin-vue/pull/2328 fixes this? There's a "logout" button in the splashscreen so the user can go back to the login page if it's endlessly loading.

pmdevita commented 6 months ago

Yep this fixes the problem. I think there's a chance requests to the server may hang and cause the loading bar and cursor to stay on for a while, but those eventually do go away and don't block any functionality. This can probably be closed now.

ferferga commented 6 months ago

@pmdevita I confirm you this chance exists ^^. Bear in mind that the caching/reactivity layer I implemented in #2201 with the client/server state is right now a "best effort". Somewhat the client is already capable of working offline but there are some places/caveats which still expect to be online (images is the most basic example that cames to my mind).

Those kind of edge cases I'm aware of and are minor annoyances that currently goes out of scope, but it's true that what you pointed in this issue is a major problem which could render the client completely unusable in many common situations (reinstalling the server, for example).

Although we still have those minor caveats, I think we have this major concern solved and the other details will be solved along while the frontend matures or in other issues, so I'm going to close this if you don't mind 👍.