Fix login page loading#6499
Conversation
🦋 Changeset detectedLatest commit: a854b6c The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6499 +/- ##
=======================================
Coverage 47.79% 47.80%
=======================================
Files 2581 2581
Lines 45451 45456 +5
Branches 10704 10675 -29
=======================================
+ Hits 21724 21730 +6
+ Misses 22490 22489 -1
Partials 1237 1237
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves the login UX by avoiding an incorrect “no login method available” empty-state while the login page is still determining which authentication methods are enabled/available.
Changes:
- Render a spinner (instead of the empty-state message) while auth configuration is loading and no login methods are known yet.
- Add a changeset entry documenting the patch change.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/auth/components/LoginPage/LoginPage.tsx |
Adds a loading-state spinner in the “no login method” area while auth config is still loading. |
.changeset/cool-trains-fetch.md |
Records the patch-level UX fix in the changelog. |
| {!passwordLoginEnabled && | ||
| (!externalAuthentications || externalAuthentications.length === 0) && | ||
| (loading || !externalAuthentications ? ( | ||
| <Box | ||
| display="flex" | ||
| justifyContent="center" | ||
| width="100%" | ||
| data-test-id="login-methods-loading" | ||
| > | ||
| <Spinner aria-label="Loading login methods" /> | ||
| </Box> |
There was a problem hiding this comment.
The spinner is rendered whenever externalAuthentications is undefined, even when loading is false. In cases where the query is skipped or errors out (so the list never becomes defined), this can result in a permanent loading indicator and no actionable feedback. Consider rendering the spinner only while loading is true, and falling back to the empty-state (or a dedicated error state) once loading has finished but the methods list is still missing.
| width="100%" | ||
| data-test-id="login-methods-loading" | ||
| > | ||
| <Spinner aria-label="Loading login methods" /> |
There was a problem hiding this comment.
nitpick: this could use intl.formattedMessage for translations
| {!passwordLoginEnabled && | ||
| (!externalAuthentications || externalAuthentications.length === 0) && | ||
| (loading || !externalAuthentications ? ( |
There was a problem hiding this comment.
this condition got pretty complicated, maybe we could refactor this into something easier to understand?
e.g.
const hasExternalAuthentications = ...
const showLoadingState = ...it also looks like we check externalAuthentications twice:
!externalAuthentications ...
(loading || !externalAuthentications ?| const hasExternalAuthentications = (externalAuthentications?.length ?? 0) > 0; | ||
| const isLoadingLoginMethods = loading || !externalAuthentications; | ||
| const showLoginMethodsSpinner = | ||
| !passwordLoginEnabled && !hasExternalAuthentications && isLoadingLoginMethods; | ||
| const showPasswordLoginDisabledMessage = | ||
| !passwordLoginEnabled && !hasExternalAuthentications && !isLoadingLoginMethods; |
There was a problem hiding this comment.
isLoadingLoginMethods treats externalAuthentications being undefined as “loading” even when the loading prop is false. This can result in a spinner that never resolves if the caller ends up with data === undefined and loading === false (e.g. GraphQL error, skipped query, or aborted request). Consider relying solely on the loading prop (and pass it correctly from the parent), or introduce an explicit "login methods loaded"/"login methods error" signal so the UI can fall back to an empty/error state instead of a perpetual spinner.
Scope of the change