Added loading spinner for Flows that take time to load - #1929
Conversation
|
🐕 Review complete — View session on Shuni Portal 🐾 🤖 Model: |
🐕 Suggested ReviewersLoading spinner feature review assignment examines JSX and CSS contributions, prioritizes component overlay and responsive styling verification
Suggested by Shuni based on git history and PR context. Names are not @-mentioned to avoid notifying anyone — request a review from whoever fits best. |
![shuni-bot[bot]](https://avatars.githubusercontent.com/in/2536750?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni Review
Adds a full-screen loading overlay/spinner shown until the Descope flow fires onReady/onError, configurable via the loading and loading_color query params (defaulting to the bg color).
Actionable comments posted: 3
- 1 🟠 Major: overlay never dismissed when
FlowGateblocks the flow, permanently hiding the error screen - 2 🟡 Minor:
http://background URL leaks into the spinner color;color-mix()border shorthand drops the whole border on invalid/unsupported values
Merge risk: 🟡 Moderate: the blocked-domain path now renders an undismissable spinner over the error screen.
See inline comments for details.
🤖 Model: claude-opus-5
Review scope: Full review
Reviewed files (6)
.env.exampleREADME.mdsrc/App.csssrc/App.test.tsxsrc/App.tsxsrc/components/FlowLoadingOverlay.tsx
| {showFlow && showFlowLoading && !isFlowReady && ( | ||
| <FlowLoadingOverlay color={loadingSpinnerColor} /> | ||
| )} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🛠️ Moderate
Dismiss the overlay when the flow never mounts
The overlay is gated only on showFlow and on readyFlowKey, which is set exclusively by <Descope>'s onReady/onError. When FlowGate gets success !== true from /v1/flow/validate-domain it renders <ErrorScreen /> instead of its children, so <Descope> never mounts and neither callback ever fires — the position: fixed; z-index: 1000 overlay then covers the error message forever with a spinning indicator and no way out. The same dead end occurs whenever the web component fails to initialize without emitting error.
Proposed fix
Either render the overlay inside FlowGate (so the blocked branch replaces it too) or have FlowGate report its blocked state up to App and include it in the overlay condition. Also add a max-wait timeout that clears the overlay so an SDK that emits neither event can't strand the page.
There was a problem hiding this comment.
Still open on the current head (2da0004). The overlay condition and FlowGate are unchanged; making the spinner opt-in (loading=true / DESCOPE_FLOW_LOADING=true) narrows the blast radius to deployments that enable it, but for those the blocked-domain path still renders ErrorScreen behind an undismissable overlay, and there is still no max-wait fallback.
| const isBackgroundImageUrl = (value: string | undefined) => | ||
| Boolean(value?.startsWith('https://')); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick fix
Exclude http:// background URLs from the spinner color
isBackgroundImageUrl only matches https://, so an http:// image URL in bg/DESCOPE_BG is treated as a color and passed through to --flow-loading-color. Because the custom property is set (just not a color), the var(..., #0082b5) fallbacks in .flow-loading-spinner never apply and the declarations become invalid at computed-value time instead of falling back to the default color.
| const isBackgroundImageUrl = (value: string | undefined) => | |
| Boolean(value?.startsWith('https://')); | |
| const isBackgroundImageUrl = (value: string | undefined) => | |
| /^https?:\/\//i.test(value ?? ''); |
There was a problem hiding this comment.
Still open on the current head (2da0004): isBackgroundImageUrl is unchanged, so an http:// value in bg/DESCOPE_BG continues to reach --flow-loading-color whenever the spinner is enabled.
| color-mix(in srgb, var(--flow-loading-color, #0082b5) 20%, transparent); | ||
| border-top-color: var(--flow-loading-color, #0082b5); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick fix
Keep the spinner visible when the color value is unusable
Putting color-mix() with a var() reference in the border shorthand makes width and style depend on the color resolving: if --flow-loading-color holds any non-color string (an arbitrary bg/loading_color value reaches it unvalidated) or the browser lacks color-mix() — still possible under the repo's >0.2%, not dead browserslist — the whole shorthand is dropped to border-style: none and the spinner renders invisible while the opaque overlay keeps blocking the flow. A static track color isolates the failure to the accent arc.
| color-mix(in srgb, var(--flow-loading-color, #0082b5) 20%, transparent); | |
| border-top-color: var(--flow-loading-color, #0082b5); | |
| border: 3px solid rgba(0, 0, 0, 0.12); |
There was a problem hiding this comment.
Still open on the current head (2da0004): src/App.css was not touched by the follow-up commits, so the border shorthand still collapses to border-style: none when the color value is invalid or color-mix() is unsupported.
|
🐕 Review complete — View session on Shuni Portal 🐾 🤖 Model: |
There was a problem hiding this comment.
🐕 Shuni Review
No new issues found.
3 unresolved prior findings
⚠️ Review follow-up issue: Unresolved prior review threads: PRRT_kwDOJrNxSs6fEmoG, PRRT_kwDOJrNxSs6fEmoL, PRRT_kwDOJrNxSs6fEmoU.
🤖 Model: claude-opus-5
Review scope: Incremental re-review
Reviewed new changes (4)
.env.exampleREADME.mdsrc/App.test.tsxsrc/App.tsx
1 participant
Related Issues
✅ Fixes https://github.com/descope/etc/issues/18199
Description
This change adds a configurable loading spinner for when flow screens take some time to load, or for when you're doing redirects via OAuth and SSO actions at the beginning of the flow.
Screenshots
Must