fix: make sure the app is fully ready before trying to redirect to the login page #1337
No reviewers
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
3 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#1337
Loading…
Reference in New Issue
No description provided.
Delete Branch "fix/app-ready"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR moves the check if the user is authenticated to the
ready
component to make sure the app is fully loaded before redirecting the user to the login page.Before this PR, Vikunja would first redirect to login and then, once everything was ready, back to the page the user wanted to visit. Because all of happened while the loading spinner overlay was visible, one would not notice.
However, when getting redirected from migrators, the get parameters of that redirect would be lost. That essentially stopped all migrators from working correctly.
Fixes #1264
@ -66,1 +72,3 @@
} catch(e: any) {
const redirectTo = checkAuth(route)
if (typeof redirectTo !== 'undefined') {
await router.push(redirectTo)
I wanted to make sure the app would only be ready after it was redirected. What bothers me here is I had to move the
store.commit('vikunjaReady', true)
out of theloadApp
action. It should be fine since this is the only place where that action is used, but I'm not quite sure about it.Can you give a bit more context what you mean by:
That was meant the other way around - "...make sure the app would only redirect after it was ready"
See my lenghty comment below.
Ahh that makes more sense. I really broke my mind on this one :D
Hi konrad!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://1337-fixapp-ready--vikunja-frontend-preview.netlify.app
You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.
Have a nice day!
@ -575,11 +575,7 @@ const router = createRouter({
],
})
router.beforeEach((to) => {
This function was meant to check the authentification on each navigation step. If we remove it the user will be able to navigate even if he shouldn't be allowed.
See: https://next.router.vuejs.org/guide/advanced/navigation-guards.html#global-before-guards
The problem with the way this was implemented is it will always redirect to
/login
while the application is not yet loaded and it does not yet know whether the user is logged in or not. Then, once it knows that, thebeforeEach
handler would allow the request and redirect to the page the user last visited (but I think the last part happens inApp.vue
or related).There are two problems with that:
/login
even though the user did not activeley navigates there. This creates problems when I re-open or reload a browser tab with Vikunja in it which has old entries and then want to navigate back to those entries. I have to go look in the browser history to get there.Example: User navigates from
/
to/1
to/2
to/3
. The history after this navigation is['/', '/1', '/2', '/3']
. Then thy reload the page with an under-the-hood redirect happening to/login
and back to/3
. The browser history now is['/', '/1', '/2', '/3', '/login', '/3']
- when the user then presses the back button and expects to get to/2
they go to/login
and get redirected immediately to/3
. This is highly confusing.While the second one would be fixable through other means, the first one would require a lot more work with little additional benefit over the solution I implemented in this PR.
Because I moved the check to the
<ready>
component instead, the application will only check whether or not to redirect the user if it is possible to know whether the user is logged in or now. This avoids the redirect to/login
and back while preventing unauthenticated users to access the app.@ -580,3 +578,1 @@
})
function checkAuth(route: RouteLocation) {
export function checkAuth(route: RouteLocation) {
Regardless of the above: I confused myself by naming this function the same way as the
checkAuth
store action.Maybe this here should be called
getAuthForRoute
What do you thinkYeah that sounds like a good idea.
Renamed.
Ah so we don't need the vikunjaReady state anymore in store because the ready component is the only place it is used?