Uh oh!
There was an error while loading. Please reload this page.
fix(ui): hide protect-check spinner while challenge widget is visible - #9099
Conversation
The protect-check card had no notion of the challenge SDK rendering visible content: the spinner stayed up alongside the Turnstile widget, and the empty SDK container reserved 60px of dead space above the spinner. Track the container's rendered height in useProtectCheckRunner (ResizeObserver) and key the spinner + container layout off it. PROT-863
Address external review findings on the spinner fix: - Attach the visibility observer via a callback ref: SignUpProtectCheck renders null until a challenge exists, so a mount-time effect could permanently miss the container. - Fall back to MutationObserver where ResizeObserver is unavailable (legacy bundle) instead of freezing the empty-state layout. - Clear the container when a new run starts, so a retry or chained challenge doesn't keep a dead widget on screen (and the SDK can't stack a second one under it). PROT-863
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 3225a44 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
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 |
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds widget visibility signaling to the Protect check SDK path, tracks that state in the UI runner, and updates SignIn and SignUp protect-check layouts and spinner behavior so the spinner hides while the widget is visible. Tests and a changeset were added. ChangesProtect check widget visibility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
API Changes Report
Summary
@clerk/sharedCurrent version: 4.24.0 Subpath |
@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/electron@clerk/electron-passkeys@clerk/eslint-plugin@clerk/expo@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/hooks/useProtectCheckRunner.ts (1)
221-232: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider synchronously resetting
isWidgetVisiblewhen clearing the container.The container is cleared here, but
isWidgetVisiblestate is left untouched — it only updates once theResizeObserver/MutationObservercallback fires.ResizeObservercallbacks run after layout but before paint in the same frame, so in practice this shouldn't produce a visible flash in browsers that support it, but theMutationObserverfallback (used in legacy browsers) has less strict scheduling guarantees, and the test suite for this feature has to manually callsetRenderedHeight(container, 0)after retry rather than asserting the reset happens automatically — a sign the state isn't deterministically synced with the DOM clear.Explicitly setting
isWidgetVisibletofalsehere would remove the dependency on observer timing and make the behavior deterministic across browsers.♻️ Proposed fix
const container = containerNodeRef.current; if (!container) { return; } // This run owns the container outright: drop anything a previous run left behind (a solved or // errored widget) so the spinner covers the load phase and a re-rendering SDK can't stack a // second widget under a stale one. while (container.firstChild) { container.removeChild(container.firstChild); } + setIsWidgetVisible(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/hooks/useProtectCheckRunner.ts` around lines 221 - 232, When `useProtectCheckRunner` clears the `containerNodeRef.current` DOM, also synchronously reset `isWidgetVisible` to false in the same cleanup path. Update the logic around the container wipe so the state is kept in sync immediately instead of waiting for the `ResizeObserver`/`MutationObserver` callbacks, using the existing `containerNodeRef` and `isWidgetVisible` state in `useProtectCheckRunner`. This makes the widget visibility deterministic across browsers and avoids relying on observer timing after a retry or rerender.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/ui/src/hooks/useProtectCheckRunner.ts`:
- Around line 221-232: When `useProtectCheckRunner` clears the
`containerNodeRef.current` DOM, also synchronously reset `isWidgetVisible` to
false in the same cleanup path. Update the logic around the container wipe so
the state is kept in sync immediately instead of waiting for the
`ResizeObserver`/`MutationObserver` callbacks, using the existing
`containerNodeRef` and `isWidgetVisible` state in `useProtectCheckRunner`. This
makes the widget visibility deterministic across browsers and avoids relying on
observer timing after a retry or rerender.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b377013b-7548-4708-a9a0-b221429b17d9
📒 Files selected for processing (6)
.changeset/protect-check-spinner-widget-visibility.mdpackages/ui/src/components/SignIn/SignInProtectCheck.tsxpackages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsxpackages/ui/src/components/SignUp/SignUpProtectCheck.tsxpackages/ui/src/components/SignUp/__tests__/SignUpProtectCheck.test.tsxpackages/ui/src/hooks/useProtectCheckRunner.ts
…container Keeps isWidgetVisible deterministically in sync with the DOM wipe instead of depending on observer scheduling (looser on the MutationObserver fallback). Review feedback from CodeRabbit on #9099. PROT-863
A spinner with visible 'Loading…' text next to it isn't a pattern the design system uses anywhere else (thread feedback from Alex Carpenter). Show just the spinner and carry the localized label as aria-label — the Spinner primitive already announces via aria-busy/aria-live, matching how Button labels its loading state. PROT-863
…ignal Replace DOM observation with an explicit contract (per Theo): the executeProtectCheck init payload now carries an optional setWidgetVisible(visible) callback. The script calls it right before revealing UI in the container (and with false once its widget is done); the promise resolves only after the host commits the change to the DOM (flushSync), so the script can reveal its widget with no overlap frame. Scripts that know their widget is imminent can call it immediately to avoid a spinner flash. The ResizeObserver/MutationObserver machinery and callback-ref plumbing in useProtectCheckRunner are gone; isWidgetVisible is purely signal-driven. Deploy note: the speed-bump script must ship its setWidgetVisible calls (guarded, clerk/protect#213) before this reaches production, or the spinner stays visible under the widget again. PROT-863
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/hooks/useProtectCheckRunner.ts`:
- Around line 78-88: The current setWidgetVisible callback in
useProtectCheckRunner can still update state after a run has been cleaned up,
letting a stale script flip the next run’s isWidgetVisible. Scope the callback
to each executeProtectCheck run or add a guard using that run’s cancelled flag
before calling flushSync, so any late setWidgetVisible calls from an abandoned
challenge become no-ops.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d1504b7-7333-4fe7-ba37-3725cf326f39
📒 Files selected for processing (6)
.changeset/protect-check-spinner-widget-visibility.mdpackages/shared/src/internal/clerk-js/__tests__/protectCheck.test.tspackages/shared/src/internal/clerk-js/protectCheck.tspackages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsxpackages/ui/src/components/SignUp/__tests__/SignUpProtectCheck.test.tsxpackages/ui/src/hooks/useProtectCheckRunner.ts
Uh oh!
There was an error while loading. Please reload this page.
alexcarpenter
left a comment
There was a problem hiding this comment.
take a look at #9099 (comment) before merging but else lgtm
The abort contract is best-effort, so a zombie script from a timed-out, retried, or superseded run could still call the stable hook-level setWidgetVisible and flip visibility under the active run. Build the callback per run inside the effect, guarded by that run's cancelled flag, its abort signal, and mount state. External review finding on #9099. PROT-863
…h it Gate the spinner's entrance behind the house useSpinDelay (300ms): a check that completes inside the window — or a script that signals its widget immediately — shows no spinner at all instead of a flash. The error and widget-visibility gates stay outside the delay hook so its minimum-visible duration can never outrank the handshake's spinner-gone-at-resolve guarantee or hold a spinner next to the retry button. Matters most once PROT-864 pre-warms Turnstile and execute() resolves in a couple hundred milliseconds. PROT-863
Uh oh!
There was an error while loading. Please reload this page.
alexcarpenter
left a comment
There was a problem hiding this comment.
one nit about changeset but LGTM
Co-authored-by: Alex Carpenter <alex.carpenter@clerk.dev>
Description
Fixes two UX issues in the Protect check card reported while dogfooding the challenge flow (Slack report):
minHeight: 60px, leaving a hole of dead space between the header and the spinner before any widget rendered.Root cause: the card only knows "the check is running" (
isRunningspans the whole lifecycle — SDK load → widget render → solve → verify → proof submit), and the challenge-script contract had no way to say "I'm showing UI now".What changed
@clerk/shared— theexecuteProtectCheckcontract gains an optional visibility handshake: the host passessetWidgetVisible(visible: boolean): Promise<void>in the script's init payload. The script calls it right before revealing UI in the container (and withfalseonce its widget is done). The promise resolves only after the host has committed the change to the DOM, so the script can reveal its widget with no overlap frame; a script that knows its widget is imminent can call it immediately so the spinner never flashes. Scripts must treat the field as optional (older hosts don't send it).packages/ui—useProtectCheckRunnerimplements the host side and exposes the signal-drivenisWidgetVisible:flushSync(the same commit-before-returning guaranteeBaseRouterrelies on), then resolves — that ordering is what makes the "no overlap frame" promise real.cancelledflag, abort signal, and mount state: aborting a script is best-effort by contract, so a zombie script from a timed-out, retried, or superseded run can still call it late — those stale signals resolve as silent no-ops instead of flipping visibility under the active run.useSpinDelayhook (300ms): a check that completes inside the window — or a script that signals its widget immediately — never flashes a spinner at all (the card header carries the first moments). The error and widget-visibility gates sit outside the delay hook, so its minimum-visible duration can never outrank the handshake's spinner-gone-at-resolve guarantee or hold a spinner next to the retry button.position: absolute, theCaptchaElementgaplessidiom) instead of reserving 60px.Spinnerasaria-label(theButtonloading idiom;Spinneralready announces viaaria-busy/aria-live).Deploy ordering
The speed-bump script's side of the handshake is clerk/protect#213 (guarded fallback, safe on older hosts). Specter deploys before this reaches production: this host listens only for the signal, so against an unsignaling old script the spinner stays under the widget and the container never re-enters layout flow.
How to test
pnpm vitest run src/components/SignIn/__tests__/SignInProtectCheck.test.tsx src/components/SignUp/__tests__/SignUpProtectCheck.test.tsxinpackages/ui(visibility swap driven through the captured signal, async-confirm semantics, layout flip, retry hygiene, zombie-run guard, spin-delay entrance and its gates), andsrc/internal/clerk-js/__tests__/protectCheck.test.tsinpackages/shared(contract forwarding).protect_checkagainst a Specter serving the fix(backend-core): Fix case of buildAuthState erroneous return value #213 script — spinner alone during invisible phases, widget alone when interaction is required, spinner back during the verify round-trip.Fixes PROT-863
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Summary
Bug Fixes
Tests