refactor(shared,ui): extract the Protect check lifecycle into shared internal helpers - #9391
refactor(shared,ui): extract the Protect check lifecycle into shared internal helpers#9391mwickett wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: c740264 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@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/vue
commit: |
API Changes Report
Summary
@clerk/sharedCurrent version: 4.29.0 Subpath
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6193d98ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Moved to the public `@clerk/shared/protect-check` entry; this subpath is kept as an alias so | ||
| // existing imports keep resolving. |
There was a problem hiding this comment.
This comment only narrates that the following re-export preserves the existing subpath, which is already evident from the code. Remove it rather than adding a multi-line restatement; repository guidance permits comments only for critical, non-obvious rationale and requires them to be terse.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change extracts Protect Check timeout, cancellation, cleanup, and proof-submission recovery into shared lifecycle helpers. The UI runner now delegates execution and submission to these helpers. Sign-in and sign-up tests mock the timeout-based executor while preserving other lifecycle implementations. New shared tests cover execution and submission states. Two changesets document patch releases. Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the shared helper signature in both mocks.
Both tests cast
executeProtectCheckWithTimeouttoReturnType<typeof vi.fn>. This removes compile-time validation of mock parameters and return values. Usevi.mocked(executeProtectCheckWithTimeout)or an equivalentMockedFunctiontype in both files.
packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx#L22-L22: Replace the broad cast with a typed Vitest mock.packages/ui/src/components/SignUp/__tests__/SignUpProtectCheck.test.tsx#L23-L23: Replace the broad cast with a typed Vitest mock.As per coding guidelines, use precise TypeScript types and avoid broad mock types when a narrower type is available.
🤖 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/components/SignIn/__tests__/SignInProtectCheck.test.tsx` at line 22, Replace the broad ReturnType<typeof vi.fn> casts for executeProtectCheckWithTimeout with vi.mocked or an equivalent precise MockedFunction type in both packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx:22-22 and packages/ui/src/components/SignUp/__tests__/SignUpProtectCheck.test.tsx:23-23, preserving compile-time validation of the shared helper’s parameters and return value.Source: Coding guidelines
packages/shared/src/protect-check/executeProtectCheck.ts (1)
59-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
PROTECT_CHECK_ERROR_CODESfor all thrown error codes inexecuteProtectCheck.ts. Replace the ten hardcoded literals at lines 60, 65, 70, 108, 121, 126, 131, 144, 148, and 155.🤖 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/shared/src/protect-check/executeProtectCheck.ts` around lines 59 - 72, Update all thrown error objects in executeProtectCheck to use the corresponding constants from PROTECT_CHECK_ERROR_CODES instead of hardcoded error-code string literals, including the validation and later failure paths identified in the comment. Preserve each error’s existing semantic code while centralizing the values through that shared constant.
🤖 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/shared/src/protect-check/executeProtectCheck.ts`:
- Around line 59-72: Update all thrown error objects in executeProtectCheck to
use the corresponding constants from PROTECT_CHECK_ERROR_CODES instead of
hardcoded error-code string literals, including the validation and later failure
paths identified in the comment. Preserve each error’s existing semantic code
while centralizing the values through that shared constant.
In `@packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx`:
- Line 22: Replace the broad ReturnType<typeof vi.fn> casts for
executeProtectCheckWithTimeout with vi.mocked or an equivalent precise
MockedFunction type in both
packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx:22-22
and
packages/ui/src/components/SignUp/__tests__/SignUpProtectCheck.test.tsx:23-23,
preserving compile-time validation of the shared helper’s parameters and return
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 307fd263-40d0-4c7f-9b07-63201917dc3d
📒 Files selected for processing (14)
.changeset/protect-check-public-entry.md.changeset/protect-check-ui-refactor.mdpackages/shared/package.jsonpackages/shared/src/internal/clerk-js/protectCheck.tspackages/shared/src/protect-check/__tests__/executeProtectCheck.test.tspackages/shared/src/protect-check/__tests__/lifecycle.test.tspackages/shared/src/protect-check/errors.tspackages/shared/src/protect-check/executeProtectCheck.tspackages/shared/src/protect-check/index.tspackages/shared/src/protect-check/lifecycle.tspackages/shared/tsdown.config.mtspackages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsxpackages/ui/src/components/SignUp/__tests__/SignUpProtectCheck.test.tsxpackages/ui/src/hooks/useProtectCheckRunner.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/cli(auto-detected)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)
f6193d9 to
b1e90f9
Compare
mwickett
changed the title
2 participants
Description
Groundwork for Protect challenge support in custom-flow (headless) apps, where the challenge experience will be fully Clerk-owned: rendered automatically (modal by default, or into a
clerk-protect-checkplacement marker), with no customer-facing challenge API — the same contract the bot-protection CAPTCHA already has.This PR extracts the framework-free half of the challenge lifecycle out of
@clerk/ui'suseProtectCheckRunnerinto@clerk/shared/internal/clerk-js/protectCheckLifecycle, so the upcoming clerk-js managed-challenge host can drive the exact same lifecycle the prebuilt components use, with no duplication. No behavioral changes and no public API changes.protectCheckLifecycleexposing:executeProtectCheckWithTimeout— container ownership (clears stale widgets), the script timeout race, one-way caller-signal linking, and the zombie-scriptsetWidgetVisibleguardsubmitProtectCheckProof— proof submission withprotect_check_already_resolvedtreated as soft success (reload + continue)isProtectCheckExpired,MAX_EXPIRED_RELOADS,PROTECT_CHECK_SCRIPT_TIMEOUT_MS@clerk/ui'suseProtectCheckRunnernow drives its lifecycle through those helpers and keeps only the React orchestration (effect keying on the challenge token, mount/cancel races,flushSyncvisibility handshake, card error state, the no-RHC fail-closed guard — which must stay in the consumer layer since the shared module compiles with__BUILD_DISABLE_RHC__hard-codedfalse).import(), so no-RHC bundles (chrome-extension,clerk.no-rhc.js) continue to dead-code-eliminate the loader entirely.executeProtectCheckitself is untouched.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change