-
Notifications
You must be signed in to change notification settings - Fork 13
Added loading spinner for Flows that take time to load #1929
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,11 +1,18 @@ | ||||||||||
| import { AuthProvider, Descope } from '@descope/react-sdk'; | ||||||||||
| import { FlowJWTResponse } from '@descope/web-component'; | ||||||||||
| import clsx from 'clsx'; | ||||||||||
| import React, { useEffect, useMemo, useCallback, CSSProperties } from 'react'; | ||||||||||
| import React, { | ||||||||||
| useEffect, | ||||||||||
| useMemo, | ||||||||||
| useCallback, | ||||||||||
| useState, | ||||||||||
| CSSProperties | ||||||||||
| } from 'react'; | ||||||||||
| import './App.css'; | ||||||||||
| import Done from './components/Done'; | ||||||||||
| import Welcome from './components/Welcome'; | ||||||||||
| import FlowGate from './components/FlowGate'; | ||||||||||
| import FlowLoadingOverlay from './components/FlowLoadingOverlay'; | ||||||||||
| import useOidcMfa from './hooks/useOidcMfa'; | ||||||||||
| import { env } from './env'; | ||||||||||
| import { logger } from './utils/logger'; | ||||||||||
|
|
@@ -25,6 +32,29 @@ const normalizeBackgroundParam = ( | |||||||||
| return value; | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| const DEFAULT_LOADING_COLOR = '#0082b5'; | ||||||||||
|
|
||||||||||
| const isBackgroundImageUrl = (value: string | undefined) => | ||||||||||
| Boolean(value?.startsWith('https://')); | ||||||||||
|
Comment on lines
+37
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick fix Exclude
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still open on the current head (2da0004): |
||||||||||
|
|
||||||||||
| const getLoadingSpinnerColor = ({ | ||||||||||
| loadingColor, | ||||||||||
| background | ||||||||||
| }: { | ||||||||||
| loadingColor: string | undefined; | ||||||||||
| background: string | undefined; | ||||||||||
| }) => { | ||||||||||
| if (loadingColor && !isBackgroundImageUrl(loadingColor)) { | ||||||||||
| return loadingColor; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (background && !isBackgroundImageUrl(background)) { | ||||||||||
| return background; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return DEFAULT_LOADING_COLOR; | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| const isFaviconUrlSecure = (url: string) => { | ||||||||||
| try { | ||||||||||
| const parsedUrl = new URL(url); | ||||||||||
|
|
@@ -294,18 +324,37 @@ const App = () => { | |||||||||
|
|
||||||||||
| const client = useMemo(() => getClientParams(urlParams), [urlParams]); | ||||||||||
|
|
||||||||||
| const flowProps = { | ||||||||||
| flowId, | ||||||||||
| debug, | ||||||||||
| sendSessionToken, | ||||||||||
| locale, | ||||||||||
| tenant: tenantId, | ||||||||||
| theme, | ||||||||||
| styleId, | ||||||||||
| form, | ||||||||||
| client, | ||||||||||
| onSuccess: (e: CustomEvent<FlowJWTResponse>) => { | ||||||||||
| const showFlowLoading = | ||||||||||
| urlParams.get('loading') === 'true' || env.DESCOPE_FLOW_LOADING === 'true'; | ||||||||||
|
|
||||||||||
| const loadingSpinnerColor = useMemo( | ||||||||||
| () => | ||||||||||
| getLoadingSpinnerColor({ | ||||||||||
| loadingColor: normalizeBackgroundParam( | ||||||||||
| urlParams.get('loading_color') || env.DESCOPE_LOADING_COLOR | ||||||||||
| ), | ||||||||||
| background | ||||||||||
| }), | ||||||||||
| [urlParams, background] | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| const showFlow = !done && Boolean(projectId && flowId); | ||||||||||
| const flowSessionKey = `${projectId}:${flowId}`; | ||||||||||
| const [readyFlowKey, setReadyFlowKey] = useState<string | null>(null); | ||||||||||
| const isFlowReady = readyFlowKey === flowSessionKey; | ||||||||||
|
|
||||||||||
| const handleFlowReady = useCallback(() => { | ||||||||||
| setReadyFlowKey(flowSessionKey); | ||||||||||
| }, [flowSessionKey]); | ||||||||||
|
|
||||||||||
| const handleFlowError = useCallback(() => { | ||||||||||
| setReadyFlowKey(flowSessionKey); | ||||||||||
| }, [flowSessionKey]); | ||||||||||
|
|
||||||||||
| const handleFlowSuccess = useCallback( | ||||||||||
| (e: CustomEvent<FlowJWTResponse>) => { | ||||||||||
| if (flowId === 'saml-config' || flowId === 'sso-config') { | ||||||||||
| setReadyFlowKey(null); | ||||||||||
| let search = window?.location.search; | ||||||||||
| if (search) { | ||||||||||
| search = `${search}&done=true`; | ||||||||||
|
|
@@ -320,10 +369,27 @@ const App = () => { | |||||||||
| return; | ||||||||||
| } | ||||||||||
| if (e?.detail?.flowOutput?.onSuccessRedirectUrl) { | ||||||||||
| setReadyFlowKey(null); | ||||||||||
| // make sure to validate the URL in the flow against approved domains | ||||||||||
| window?.location.assign(e?.detail?.flowOutput?.onSuccessRedirectUrl); | ||||||||||
| } | ||||||||||
| }, | ||||||||||
| [flowId] | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| const flowProps = { | ||||||||||
| flowId, | ||||||||||
| debug, | ||||||||||
| sendSessionToken, | ||||||||||
| locale, | ||||||||||
| tenant: tenantId, | ||||||||||
| theme, | ||||||||||
| styleId, | ||||||||||
| form, | ||||||||||
| client, | ||||||||||
| onReady: handleFlowReady, | ||||||||||
| onError: handleFlowError, | ||||||||||
| onSuccess: handleFlowSuccess, | ||||||||||
| ...((flowId === 'saml-config' || flowId === 'sso-config') && { | ||||||||||
| autoFocus: false | ||||||||||
| }) | ||||||||||
|
|
@@ -337,14 +403,17 @@ const App = () => { | |||||||||
| persistTokens={persistTokens} | ||||||||||
| > | ||||||||||
| <div className="app" style={bodyCss} data-testid="app"> | ||||||||||
| {!done && projectId && flowId && ( | ||||||||||
| {showFlow && showFlowLoading && !isFlowReady && ( | ||||||||||
| <FlowLoadingOverlay color={loadingSpinnerColor} /> | ||||||||||
| )} | ||||||||||
|
Comment on lines
+405
to
+407
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🛠️ Moderate Dismiss the overlay when the flow never mounts The overlay is gated only on Proposed fixEither render the overlay inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still open on the current head (2da0004). The overlay condition and |
||||||||||
| {showFlow && ( | ||||||||||
| <div | ||||||||||
| className={containerClasses} | ||||||||||
| style={containerCss} | ||||||||||
| data-testid="descope-component" | ||||||||||
| > | ||||||||||
| <FlowGate baseUrl={baseUrl} projectId={projectId}> | ||||||||||
| <Descope {...flowProps} /> | ||||||||||
| <Descope key={flowSessionKey} {...flowProps} /> | ||||||||||
| </FlowGate> | ||||||||||
| </div> | ||||||||||
| )} | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import React, { CSSProperties } from 'react'; | ||
| import '../App.css'; | ||
|
|
||
| type FlowLoadingOverlayProps = { | ||
| color: string; | ||
| }; | ||
|
|
||
| const FlowLoadingOverlay = ({ color }: FlowLoadingOverlayProps) => { | ||
| const spinnerStyle = { | ||
| '--flow-loading-color': color | ||
| } as CSSProperties; | ||
|
|
||
| return ( | ||
| <div | ||
| className="flow-loading-overlay" | ||
| data-testid="flow-loading-overlay" | ||
| role="status" | ||
| aria-live="polite" | ||
| aria-busy="true" | ||
| aria-label="Loading" | ||
| > | ||
| <div | ||
| className="flow-loading-spinner" | ||
| style={spinnerStyle} | ||
| data-testid="flow-loading-spinner" | ||
| aria-hidden="true" | ||
| /> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default FlowLoadingOverlay; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick fix
Keep the spinner visible when the color value is unusable
Putting
color-mix()with avar()reference in thebordershorthand makes width and style depend on the color resolving: if--flow-loading-colorholds any non-color string (an arbitrarybg/loading_colorvalue reaches it unvalidated) or the browser lackscolor-mix()— still possible under the repo's>0.2%, not deadbrowserslist — the whole shorthand is dropped toborder-style: noneand the spinner renders invisible while the opaque overlay keeps blocking the flow. A static track color isolates the failure to the accent arc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still open on the current head (2da0004):
src/App.csswas not touched by the follow-up commits, so thebordershorthand still collapses toborder-style: nonewhen the color value is invalid orcolor-mix()is unsupported.