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
|
🐕 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
orius123
left a comment
orius123
left a comment
There was a problem hiding this comment.
Ran the branch locally against a real project and captured the states. Works as documented, spinner shows only with loading=true. 5 comments inline, the timeout one is the only real blocker for me.
- Dismiss the overlay on a timeout as well as onReady/onError, so a blocked CDN no longer leaves an unclickable fixed overlay forever. Configurable via the loading_timeout param (seconds) or DESCOPE_LOADING_TIMEOUT_MS (ms), default 15s. - Stop falling back to bg for the spinner color, which rendered a white spinner on a white page for bg=ffffff. loading_color or the default. - Tint the overlay from bg when bg is a color, so dark pages no longer flash white for the whole load. - Respect prefers-reduced-motion for the spinner animation. - Let the Descope test mock skip onReady, and cover the timeout path. - Move the loading helpers into src/shared/flowLoading.ts with unit tests, and document loading_timeout and the overlay tint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The spinner put color-mix() inside the border shorthand, so width and style depended on the color resolving. When color-mix() is unsupported (the production browserslist still resolves to ios_saf 11 and 15.6-15.8, and the build does not polyfill it) or the custom property holds a non-color, the declaration is dropped or invalid at computed-value time and border-style falls to none, hiding the spinner while the overlay kept blocking the page. Drop color-mix() from the spinner entirely: splitting it out is not enough, cssnano merges the longhands back into a color-bearing shorthand. Only the accent arc reads the custom property now. The overlay keeps an opaque var() background ahead of its color-mix() so an unsupported color-mix() no longer leaves an invisible layer that still swallows clicks. Validate colors before they reach a custom property. The var() fallback cannot rescue a property that is set but not to a color, and the old check only rejected an https:// prefix, so an http:// image URL in bg/DESCOPE_BG (or any other non-color) was passed straight through. Use CSS.supports where available and a pattern that still rejects URLs where it is not, such as jsdom. Gate the overlay on the domain check as well. FlowGate unmounts the flow when the domain is not approved, so neither onReady nor onError can fire, and the overlay sat over the error screen until the timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🐕 Shuni ReviewAdds timeout-based dismissal, domain-gate coordination, and configurable loading-overlay colors. Actionable comments posted: 1
Merge risk: 🟢 Low: a bounded rendering regression affects the loading overlay on older browsers. See inline comments for details. 🤖 Model: Review scope: Full review Reviewed files (9)
🐕 Review complete — View session on Shuni Portal 🐾 🤖 Model: |
| background: color-mix( | ||
| in srgb, | ||
| var(--flow-loading-overlay-color, #ffffff) 88%, | ||
| transparent | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick fix
Guard the color-mix() background with a feature query
On Safari versions without color-mix(), the var() in this declaration defers validation until computed-value time. It therefore overrides the preceding solid background before becoming invalid, resetting the background to transparent rather than restoring that fallback. With loading=true, the overlay loses its tint while still intercepting clicks over the visible content. Keep the solid declaration unconditional and place this enhancement inside a top-level @supports (background: color-mix(in srgb, white, transparent)) block; the feature-test value must not contain var().
2 participants
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