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(oidc): Display spinner in OIDC login button during loading #1840

Merged
merged 4 commits into from
Jan 31, 2025

Conversation

Ninfa-Jeon
Copy link
Contributor

Done

  • Fixes the phenomenon where the OIDC button was being shown after redirecting from login
  • Shows a spinner within the button and disables it while authentication is in progress

QA

  • Try logging in from with a JIMM controller
  • After redirection, the button should not be interactive and show spinner

Details

https://warthogs.atlassian.net/browse/WD-18340

Screenshots

Screenshot 2025-01-28 at 3 38 50 PM

@webteam-app
Copy link

Copy link

github-actions bot commented Jan 28, 2025

Coverage Report

Status Category Percentage Covered / Total
🟢 Lines 98.02% (🎯 95%) 14620 / 14914
🟢 Statements 98.02% (🎯 95%) 14620 / 14914
🟢 Functions 98.18% (🎯 95%) 595 / 606
🟢 Branches 92.06% (🎯 90%) 2935 / 3188
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/components/LogIn/OIDCForm/OIDCForm.tsx 100% 100% 100% 100%
src/store/general/selectors.ts 100% 100% 100% 100%
src/store/general/slice.ts 100% 85% 100% 100%
src/store/middleware/check-auth.ts 95.95% 94.73% 100% 95.95% 35-39
src/store/middleware/model-poller.ts 97.84% 92.77% 100% 97.84% 48-49, 138, 140-141, 190, 388
Generated in workflow #17 for commit de61bee by the Vitest Coverage Report Action

@Ninfa-Jeon
Copy link
Contributor Author

Ninfa-Jeon commented Jan 28, 2025

  • Making use of local storage, as I noticed it was being wiped out on every log out action.
  • Dispatching additional events before and after whoami thunk to trigger a loading state
  • Additionally, instead of hiding the button/dialog box altogether, I chose to show a spinner which reduced complexity a lot.
  • The text inside the button while loading is inspired from the IdentityProviderForm component spinner

@huwshimi
Copy link
Contributor

I think we could improve this a bit if we use the whoami request to set the loading state. It will be a little bit more work, but this will also make it a bit more generic which will help with some upcoming work to abstract our authentication.

At the moment the general state has loginErrors but I think we could make that an object like this:

login: {
  errors: string
  loading: boolean
}

Then in the general slice we could use extraReducers to update the loading state using the whoami pending/rejected/fullfilled actions.

@Ninfa-Jeon
Copy link
Contributor Author

I think we could improve this a bit if we use the whoami request to set the loading state. It will be a little bit more work, but this will also make it a bit more generic which will help with some upcoming work to abstract our authentication.

At the moment the general state has loginErrors but I think we could make that an object like this:

login: {
  errors: string
  loading: boolean
}

Then in the general slice we could use extraReducers to update the loading state using the whoami pending/rejected/fullfilled actions.

I tried doing this, but the pending state is not lasting as long as we would like the loader/spinner to show. The button is still visible as the state quickly changes from pending to fulfilled. Investigating if there is something we can do.

@huwshimi
Copy link
Contributor

I tried doing this, but the pending state is not lasting as long as we would like the loader/spinner to show. The button is still visible as the state quickly changes from pending to fulfilled. Investigating if there is something we can do.

Ah OK, in that case we could dispatch additional events at the start/end of the whoami thunk to update the loading state.

@Ninfa-Jeon Ninfa-Jeon force-pushed the show-spinner-oidc-login branch from 0732e04 to 74590de Compare January 30, 2025 08:37
@Ninfa-Jeon
Copy link
Contributor Author

I tried doing this, but the pending state is not lasting as long as we would like the loader/spinner to show. The button is still visible as the state quickly changes from pending to fulfilled. Investigating if there is something we can do.

Ah OK, in that case we could dispatch additional events at the start/end of the whoami thunk to update the loading state.

I tried various combinations for the spots we could dispatch such events and the current arrangement makes sure there is no glitch.

Copy link
Contributor

@huwshimi huwshimi left a comment

Choose a reason for hiding this comment

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

I've made some suggestions here for waiting until the user data has been retrieved.

There are two events happening, firstly the whoami check occurs to see if the user is authenticated, then the controller connection is created which is used to get the user details.

As you rightly realised we need to wait for both these events have happened otherwise it shows the login button again.

@Ninfa-Jeon Ninfa-Jeon force-pushed the show-spinner-oidc-login branch from 74590de to 374bd58 Compare January 31, 2025 05:03
Copy link
Contributor

@huwshimi huwshimi left a comment

Choose a reason for hiding this comment

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

This works really well now! Just one small comment.

@Ninfa-Jeon Ninfa-Jeon force-pushed the show-spinner-oidc-login branch from 374bd58 to de61bee Compare January 31, 2025 05:23
@Ninfa-Jeon Ninfa-Jeon merged commit 3b25638 into canonical:main Jan 31, 2025
8 checks passed
@Ninfa-Jeon Ninfa-Jeon deleted the show-spinner-oidc-login branch January 31, 2025 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants