poc: observable signin resource #6078
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
📝 Walkthrough## Walkthrough
This update introduces a reactive state management system for Clerk resources using Zustand, refactoring core resource classes and adding new store APIs and hooks. It implements a detailed observable sign-in component for debugging, updates package dependencies, and enhances error handling and async state tracking throughout the sign-in flow. New React hooks and TypeScript interfaces are provided for seamless integration.
## Changes
| File(s) | Change Summary |
|----------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| packages/clerk-js/package.json<br>packages/react/package.json | Added `[email protected]` as a runtime dependency. |
| packages/clerk-js/sandbox/app.ts<br>packages/clerk-js/sandbox/template.html | Added a "Sign In Observable" component and navigation link for interactive sign-in state visualization and debugging in the sandbox app. Introduced new route and UI controls. |
| packages/clerk-js/src/core/resources/Base.ts | Refactored `BaseResource` to integrate a Zustand store for resource state, adding a `store` property, dispatching state actions on fetch/mutate, and a `reset` method. Enhanced fetch options. |
| packages/clerk-js/src/core/resources/SignIn.ts | Refactored `SignIn` to use a combined Zustand store for state management, added `signInError`, improved async error handling, and centralized status/error updates. |
| packages/clerk-js/src/core/resources/Verification.ts | Refactored `Verification` to integrate Zustand state management for error handling, updated constructors and methods for reactive error state, and simplified initialization logic. |
| packages/clerk-js/src/core/resources/state.ts | Introduced a generic Zustand resource state management module with state unions, actions, selectors, and store creation utilities. |
| packages/clerk-js/src/ui/components/SignIn/SignInStart.ts | Removed `useLoadingStatus` hook, replaced with direct status tracking and logging, updated effect dependencies, and refined UI rendering based on new status logic. |
| packages/types/src/resource.ts | Added `ResourceStoreApi` interface for minimal reactive store API and extended `ClerkResource` with a `store` property. |
| packages/types/src/signIn.ts | Added `signInError` property to `SignInResource` interface. |
| packages/react/src/hooks/index.ts | Exported `createResourceStoreHooks` from `./useResourceStore`. |
| packages/react/src/hooks/useResourceStore.ts | Added `createResourceStoreHooks` for React integration with Zustand resource stores, providing granular hooks for state, data, error, and status. |
| packages/react/src/hooks/useSignIn.ts | Refactored `useSignIn` to remove explicit `isLoaded` flag, implementing queued method calls on proxies for `signIn` and `setActive` until the client context is ready, ensuring async readiness and adding observable state subscription. |
| packages/vue/src/composables/useSignIn.ts | Changed `useSignIn` composable to return proxies that queue method calls when Clerk client is unavailable, removing conditional loading state rendering. |
| packages/clerk-js/bundlewatch.config.json | Increased maximum allowed bundle sizes for several `clerk` JS distribution files. |
## Suggested reviewers
- tmilewski✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx (2)
207-274:⚠️ Potential issueRemove all debugging console.log statements from the useEffect.
This effect contains extensive debugging logs that should be removed.
Apply this diff to clean up the debugging statements:
useEffect(() => { - console.log('useEffect triggered'); - console.log('organizationTicket:', organizationTicket); - console.log('signInFetchStatus:', signInFetchStatus); - console.log('signInStatus:', signInStatus); - if (!organizationTicket || signInFetchStatus === 'fetching' || signInStatus === 'complete') { - console.log('Early return from useEffect'); return; } if (clerkStatus === 'sign_up') { const paramsToForward = new URLSearchParams(); if (organizationTicket) { paramsToForward.set('__clerk_ticket', organizationTicket); } - console.log('Navigating to signUpUrl with params:', paramsToForward.toString()); void navigate(isCombinedFlow ? `create` : signUpUrl, { searchParams: paramsToForward }); return; } - console.log('Setting card to loading state'); card.setLoading(); signIn .create({ strategy: 'ticket', ticket: organizationTicket, }) .then(res => { - console.log('API response:', res); switch (res.status) { case 'needs_first_factor': - console.log('Status: needs_first_factor'); if (hasOnlyEnterpriseSSOFirstFactors(res)) { - console.log('Authenticating with Enterprise SSO'); return authenticateWithEnterpriseSSO(); } return navigate('factor-one'); case 'needs_second_factor': - console.log('Status: needs_second_factor'); return navigate('factor-two'); case 'complete': - console.log('Status: complete'); removeClerkQueryParam('__clerk_ticket'); return clerk.setActive({ session: res.createdSessionId, redirectUrl: afterSignInUrl, }); default: { - console.error('Invalid API response status:', res.status); console.error(clerkInvalidFAPIResponse(res.status, supportEmail)); return; } } }) .catch(err => { - console.error('Error during signIn.create:', err); return attemptToRecoverFromSignInError(err); }) .finally(() => { const isRedirectingToSSOProvider = hasOnlyEnterpriseSSOFirstFactors(signIn); if (isRedirectingToSSOProvider) return; - console.log('Setting card to idle state'); card.setIdle(); }); }, [organizationTicket, signInFetchStatus, signInStatus]);
276-320:⚠️ Potential issueRemove console.log statements from OAuth error handling.
The OAuth error handling effect also contains debugging logs that should be cleaned up.
Apply this diff:
- console.log('OAuth error handling useEffect triggered'); async function handleOauthError() { const defaultErrorHandler = () => { - console.error('Default error handler triggered'); card.setError('Unable to complete action at this time. If the problem persists please contact support.'); }; const error = signIn?.firstFactorVerification?.error; if (error) { - console.log('OAuth error detected:', error); switch (error.code) { // ... cases ... default: defaultErrorHandler(); } // TODO: This is a workaround in order to reset the sign in attempt // so that the oauth error does not persist on full page reloads. - console.log('Resetting sign-in attempt'); void (await signIn.create({})); } }
🧹 Nitpick comments (8)
packages/clerk-js/src/core/resources/__tests__/Environment.test.ts (1)
283-329: Consider extracting common test patterns into test utilities.The explicit assertions are good, but there's repetition in the assertion patterns across this and other resource test files. Consider creating shared test utilities for common assertion patterns to reduce duplication and improve consistency.
Would you like me to help create shared test utilities for these common resource assertion patterns?
packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx (1)
527-530: Consider a more polished UI for production use.The "Sign-in complete!" message appears to be a placeholder. Consider implementing a proper completion UI or redirecting to the appropriate page after successful sign-in.
Would you like me to suggest a more polished completion UI component?
packages/clerk-js/src/core/resources/state.ts (1)
8-12: Consider adding a 'fetching' state for clearer loading semantics.The current state types use 'loading' which might be confused with initial loading. Consider using 'fetching' to make it clearer that this represents an active fetch operation.
This is just a naming suggestion for better clarity. The current implementation works well.
packages/clerk-js/sandbox/app.ts (1)
563-578: Consider simplifying the async route handler.The route handler has duplicate Clerk loading checks. Since
Clerk.load()is called before the route handler, the additional check might be redundant.Consider verifying if the additional loading check is necessary, or if it can be handled by the main Clerk.load() call.
packages/clerk-js/src/core/resources/Base.ts (2)
83-97: Duplicated status mapping invites drift
fetchStatusmanually maps internal state → public strings. Whenever you extendResourceStateyou must update this switch or risk silent fall-through to'idle'. A safer, future-proof pattern:const map = { idle: 'idle', loading: 'fetching', success: 'fetched', error: 'error', } as const; return map[stateType];or expose the raw
state.typeand let consumers translate.
308-312:reset()only clears the Zustand store, leaving instance fields staleAfter calling
reset()the resource’s own properties (id,status, etc.) still hold the last successful values, so subsequent UI reads can show contradictory information. Either:
- Re-instantiate the object (
return new (this.constructor as any)()), or- Explicitly null/reset critical fields inside
reset().packages/clerk-js/src/core/resources/SignIn.ts (2)
72-88: Unusedtypefield & unconditional devtools
type: 'idle' | 'loading' | 'error' | 'success'is never updated, so the store always reports'idle'. Remove it or wire it properly.Additionally,
devtoolsmiddleware is enabled for every instance (including production bundles) which adds ~1 KB and leaks store snapshots to the Redux DevTools extension. Wrap it:const withDevtools = process.env.NODE_ENV === 'development' ? devtools : (f => f); create<SignInUIState>()(withDevtools(set => ({ … })));
204-231: Partial error propagation
attemptFirstFactorpushes global errors tosignInErrorbut does not propagate field-specific errors returned by the API (they exist undererrors[0].meta?.param_name). Exposing them would enable granular UI feedback.Optional enhancement: extend
updateErrorto parse and mapClerkAPIErrorJSON[]intofields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
packages/clerk-js/src/core/resources/__tests__/__snapshots__/Client.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/Environment.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/Organization.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/OrganizationDomain.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/OrganizationInvitation.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/OrganizationMembership.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/OrganizationMembershipRequest.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/OrganizationSuggestion.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/PublicUserData.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/Session.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/UserSettings.test.ts.snapis excluded by!**/*.snappackages/clerk-js/src/core/resources/__tests__/__snapshots__/Waitlist.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
packages/clerk-js/package.json(1 hunks)packages/clerk-js/sandbox/app.ts(6 hunks)packages/clerk-js/sandbox/template.html(1 hunks)packages/clerk-js/src/core/resources/Base.ts(4 hunks)packages/clerk-js/src/core/resources/SignIn.ts(4 hunks)packages/clerk-js/src/core/resources/Verification.ts(5 hunks)packages/clerk-js/src/core/resources/__tests__/Client.test.ts(2 hunks)packages/clerk-js/src/core/resources/__tests__/Environment.test.ts(3 hunks)packages/clerk-js/src/core/resources/__tests__/Organization.test.ts(1 hunks)packages/clerk-js/src/core/resources/__tests__/OrganizationDomain.test.ts(2 hunks)packages/clerk-js/src/core/resources/__tests__/OrganizationInvitation.test.ts(1 hunks)packages/clerk-js/src/core/resources/__tests__/OrganizationMembership.test.ts(2 hunks)packages/clerk-js/src/core/resources/__tests__/OrganizationMembershipRequest.test.ts(1 hunks)packages/clerk-js/src/core/resources/__tests__/OrganizationSuggestion.test.ts(1 hunks)packages/clerk-js/src/core/resources/__tests__/PublicUserData.test.ts(1 hunks)packages/clerk-js/src/core/resources/__tests__/Session.test.ts(2 hunks)packages/clerk-js/src/core/resources/__tests__/UserSettings.test.ts(1 hunks)packages/clerk-js/src/core/resources/__tests__/Waitlist.test.ts(1 hunks)packages/clerk-js/src/core/resources/state.ts(1 hunks)packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx(5 hunks)packages/types/src/resource.ts(1 hunks)packages/types/src/signIn.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/clerk-js/src/core/resources/state.ts (1)
packages/types/src/json.ts (1)
ClerkAPIErrorJSON(337-353)
packages/clerk-js/src/core/resources/Base.ts (3)
packages/types/src/resource.ts (1)
ClerkResourceReloadParams(1-3)packages/clerk-js/src/core/resources/state.ts (2)
ResourceStore(26-41)createResourceStore(59-91)packages/types/src/json.ts (1)
ClerkAPIErrorJSON(337-353)
packages/clerk-js/src/core/resources/Verification.ts (6)
packages/types/src/phoneCodeChannel.ts (1)
PhoneCodeChannel(9-9)packages/types/src/verification.ts (1)
VerificationStatus(26-26)packages/clerk-js/src/core/resources/Base.ts (2)
error(75-77)data(79-81)packages/types/src/api.ts (1)
ClerkAPIError(4-33)packages/shared/src/error.ts (2)
errorToJSON(102-115)parseError(87-100)packages/types/src/json.ts (1)
VerificationJSON(311-322)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (28)
packages/clerk-js/package.json (1)
81-81:✅ Verification successful
Verify the zustand version for security and currency.
Please ensure that version 5.0.5 is the latest stable version and check for any security advisories.
🌐 Web query:
What is the latest version of zustand and are there any known security vulnerabilities in version 5.0.5?💡 Result:
Latest Zustand Version
The latest official release of Zustand is v5.0.5, published on May 21, 2025. This release brings minor improvements and a few new methods, including updates to the shallow compare function, new devtools cleanup features, and some internal refactoring and type improvements[1]. There are no major new features in this release; rather, it focuses on dropping deprecated code and ensuring compatibility with modern React and TypeScript versions[1][7][10].
Security Vulnerabilities in v5.0.5
There are currently no publicly reported security vulnerabilities specific to Zustand v5.0.5 in official documentation or discussions[1][7]. The changelogs and migration guides highlight breaking changes and improvements, but do not list any security issues or CVEs associated with this version[1][10].
As of the latest available information:
- Zustand v5.0.5 is considered safe to use for production in terms of reported vulnerabilities[1].
- No security advisories or warnings have been published in the main repository or in widely referenced React/JavaScript ecosystem discussions[1][7].
- The focus of v5.x releases has been modernization, with deprecated features removed and stricter dependency/version requirements, further reducing exposure to known older issues[7][10].
Summary Table
Category Status/Details Latest Version v5.0.5 (May 21, 2025) Security in v5.0.5 No known vulnerabilities If you are migrating from an earlier version, review the official migration guide for best practices and breaking changes[10]. For ongoing security monitoring, regularly check the [Zustand GitHub releases page][1] and security advisories on npm and GitHub.
Citations:
- 1: https://github.com/pmndrs/zustand/releases
- 2: https://github.com/pmndrs/zustand
- 3: https://dev.to/hijazi313/state-management-in-2025-when-to-use-context-redux-zustand-or-jotai-2d2k
- 4: https://dev.to/joodi/modern-react-state-management-in-2025-a-practical-guide-2j8f
- 5: https://javascript.plainenglish.io/zustand-the-simple-and-efficient-state-management-for-react-c07dc5ee0d05
- 6: https://support.atlassian.com/confluence/kb/security-vulnerabilities-instance-health-check-failing/
- 7: https://pmnd.rs/blog/announcing-zustand-v5
- 8: https://swansoftwaresolutions.com/mobx-vs-redux-vs-zustand-which-state-manager-to-choose-in-2025/
- 9: RFC: How is v5-alpha/beta? pmndrs/zustand#2329
- 10: https://zustand.docs.pmnd.rs/migrations/migrating-to-v5
Zustand v5.0.5 is up-to-date and has no known vulnerabilities
The addition of
packages/clerk-js/package.json (line 81):"zustand": "5.0.5"is correct—v5.0.5 is the latest stable release (May 21, 2025) and no security advisories have been reported. No further changes are needed.
packages/types/src/resource.ts (1)
21-21:✅ Verification successful
Verify that implementing classes provide the fetchStatus property.
The fetchStatus property addition looks good and follows standard async operation state patterns. However, this is a breaking change to the interface.
🏁 Script executed:
#!/bin/bash # Description: Verify that BaseResource and other implementing classes provide fetchStatus property # Search for classes that implement ClerkResource and check if they have fetchStatus ast-grep --pattern $'class $_ implements $$$ClerkResource$$$ { $$$ }' # Also search for fetchStatus implementation in resource files rg -A 3 "fetchStatus" --type ts packages/clerk-js/src/core/resources/Length of output: 495
fetchStatus implementation verified in BaseResource
- Verified that
packages/clerk-js/src/core/resources/Base.tsprovides apublic get fetchStatus(): 'idle' | 'fetching' | 'fetched' | 'error'getter, satisfying the newClerkResourceinterface requirement.This addition follows standard async-state patterns and all implementing classes inherit it via
BaseResource. Approved.packages/types/src/signIn.ts (1)
92-92:✅ Verification successful
Verify that the SignIn class implements the signInError property.
The signInError property design is excellent - it properly separates global errors from field-specific validation errors. However, this is a breaking change to the interface.
🏁 Script executed:
#!/bin/bash # Description: Verify that SignIn class implements the signInError property # Search for SignIn class implementation and check for signInError property ast-grep --pattern $'class SignIn $$$implements$$$ SignInResource { $$$ }' # Search for signInError in SignIn implementation rg -A 5 "signInError" packages/clerk-js/src/core/resources/SignIn.tsLength of output: 668
SignIn class now implements the new signInError property
Verified that in packages/clerk-js/src/core/resources/SignIn.ts the
SignInclass defines and initializes:
signInError: { global: string | null; fields: Record<string, string> } = { global: null, fields: {} };- Updates it via
this.signInError = { global: globalError, fields: fieldErrors };The interface change is fully implemented—approving.
packages/clerk-js/src/core/resources/__tests__/Waitlist.test.ts (1)
12-16: Excellent improvement to test clarity.Replacing the snapshot assertion with explicit property checks makes the test more maintainable and clearly shows what's being validated - the proper conversion of timestamps to Date objects.
packages/clerk-js/src/core/resources/__tests__/PublicUserData.test.ts (1)
16-25: Good improvement to test explicitness.This change makes the test self-documenting by explicitly showing the expected camelCase property transformation (first_name → firstName, etc.). Much better than a snapshot for understanding the actual behavior being tested.
packages/clerk-js/sandbox/template.html (1)
191-197: Navigation item addition looks good.The new "Sign In Observable" navigation item follows the established pattern and is appropriately positioned between related sign-in functionality.
packages/clerk-js/src/core/resources/__tests__/OrganizationSuggestion.test.ts (1)
20-32: Excellent improvement from snapshot to explicit assertions.This change enhances test maintainability by explicitly verifying the expected object structure and property values instead of relying on fragile snapshots.
packages/clerk-js/src/core/resources/__tests__/OrganizationMembership.test.ts (2)
5-5: Good catch on the variable naming fix.The variable name correction improves code clarity.
43-72: Excellent test assertion improvements.The migration from snapshot to explicit object matching significantly improves test maintainability and clearly documents the expected object structure with comprehensive property validation.
packages/clerk-js/src/core/resources/__tests__/Organization.test.ts (1)
22-38: Solid improvement to test assertions.The explicit object matching clearly defines expected behavior and improves test maintainability compared to snapshot testing.
packages/clerk-js/src/core/resources/__tests__/Session.test.ts (2)
50-62: Excellent explicit event verification.The detailed verification of the 'token:update' event payload with structured JWT claims significantly improves test clarity over snapshot matching.
111-124: Comprehensive event dispatch validation.The explicit verification of both 'token:update' and 'session:tokenResolved' events ensures the reactive state management works correctly with proper event payloads.
packages/clerk-js/src/core/resources/__tests__/OrganizationInvitation.test.ts (1)
19-31: Excellent improvement from snapshot to explicit assertions.The replacement of
toMatchSnapshot()withtoMatchObject()makes the test more focused and less brittle. The explicit property checks clearly document the expected object structure and properly handle date type validation.packages/clerk-js/src/core/resources/__tests__/OrganizationMembershipRequest.test.ts (1)
23-36: Well-structured explicit assertions improve test clarity.The test properly validates both top-level properties and nested
publicUserDatastructure usingexpect.objectContaining(). This approach is more maintainable than snapshot testing and clearly documents the expected object shape.packages/clerk-js/src/core/resources/__tests__/UserSettings.test.ts (1)
7-24: Comprehensive validation of UserSettings default structure.The explicit assertions provide clear documentation of the expected default UserSettings structure, including nested
actionsandattributesconfigurations. This is a significant improvement over snapshot testing for understanding the object's expected shape.packages/clerk-js/src/core/resources/__tests__/OrganizationDomain.test.ts (2)
24-38: Proper validation of OrganizationDomain structure with verification object.The explicit assertions correctly validate the expected properties including the nested verification object structure with proper date type checking for
expiresAt.
56-65: Correct handling of nullable verification property.The test properly validates the scenario where
verificationandaffiliationEmailAddressare null, ensuring the object structure is validated in both presence and absence cases.packages/clerk-js/src/core/resources/__tests__/Client.test.ts (2)
271-291: Comprehensive validation of Client object structure.The explicit assertions properly validate the complex Client structure including nested
signUpandsignInobjects with their verification states. The use ofexpect.objectContaining()provides appropriate flexibility for nested object validation.
427-448: Correct snapshot structure validation with proper type handling.The test appropriately validates the snapshot structure, correctly expecting
Numbertypes for timestamps in the snapshot format versusDateobjects in the main instance. This distinction is important for snapshot serialization.packages/clerk-js/src/core/resources/__tests__/Environment.test.ts (1)
9-43: LGTM! Improved test clarity with explicit assertions.The switch from snapshot testing to explicit property assertions using
toMatchObjectimproves test maintainability and clarity. The assertions now clearly document the expected shape and default values of the Environment resource.packages/clerk-js/src/core/resources/state.ts (2)
1-4: LGTM! Well-structured reactive state management implementation.The implementation uses Zustand effectively to create a reusable pattern for managing resource states with TypeScript generics. The devtools integration is helpful for debugging.
59-91: Excellent implementation with proper separation of concerns.The use of selectors and the devtools integration with a named store makes this very debuggable. The state machine pattern with the reducer-like dispatch is clean and maintainable.
packages/clerk-js/src/core/resources/Verification.ts (4)
28-35: Good property organization and consistent initialization.The reordering of properties improves readability by grouping related fields together.
46-50: Clean integration with the reactive store.The
updateErrormethod properly transforms the error to JSON format before dispatching to the store, maintaining consistency with the store's expected format.
51-72: Proper null handling and early return pattern.The early return for null data and the use of nullish coalescing operators improve the robustness of the deserialization logic.
122-126: Good use of nullish coalescing for cleaner constructors.The simplified constructors using
??operator are more concise and readable.Also applies to: 144-145
packages/clerk-js/sandbox/app.ts (1)
1-2: Well-implemented POC demonstrating reactive sign-in functionality.The new
signInObservablecomponent effectively demonstrates the reactive state management capabilities. The UI provides clear visibility into the sign-in state, fetch status, and errors, making it an excellent proof of concept.A few observations:
- Good error handling and loading states
- Clear visual feedback with color-coded status indicators
- Proper initialization flow with Clerk loading checks
Also applies to: 39-39, 99-99, 263-485
packages/clerk-js/src/core/resources/Base.ts (1)
57-61: Generic argumentthismight not behave as intended in all subclassesPassing
thisas a type-parameter (createResourceStore<this>()) relies on TS’s contextualthistype resolving to the instance type of every future subclass. This works today, but breaks if a subclass widens/narrowsthis, uses intersection/conditional types, or is transpiled withnoImplicitThisquirks. Consider switching to a protected generic hook inside each subclass or:protected _store = createResourceStore<this & BaseResource>();to anchor the reference explicitly.
| console.log('needs_identifier'); | ||
| console.log('res.supportedFirstFactors:', res.supportedFirstFactors); |
There was a problem hiding this comment.
Remove debugging logs from signInWithFields function.
case 'needs_identifier':
- console.log('needs_identifier');
- console.log('res.supportedFirstFactors:', res.supportedFirstFactors);
// Check if we need to initiate an enterprise sso flow📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('needs_identifier'); | |
| console.log('res.supportedFirstFactors:', res.supportedFirstFactors); | |
| case 'needs_identifier': | |
| // Check if we need to initiate an enterprise sso flow |
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx at lines 391 to
392, remove the console.log statements used for debugging inside the
signInWithFields function to clean up the code and avoid unnecessary logging in
production.
jacekradko
force-pushed
the
poc/observable-signin
branch
from
de223bf to
6cbb936
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/react/src/hooks/useSignIn.ts (5)
147-152: Missing method-existence guard inprocessQueue
(targetObj as any)[method]is invoked without checking the member actually exists or is a function. A malformed call string will crash the queue handler.The previous review suggested adding a validation guard; it’s still absent.
52-70: 🛠️ Refactor suggestioncreateStoreObservable is still a hook that hides from eslint-plugin-react-hooks
The returned arrow function calls
useStore, but its name doesn’t start withuse*.
That keeps the linter blind, and if consumers callsignIn.observable()conditionally it will violate the rules of hooks at runtime just as before.Please rename the returned function to start with
use(e.g.useSignInStore) and expose it as a dedicated hook rather than a method to avoid silent misuse.
This was raised in a previous review and remains unresolved.
140-157:⚠️ Potential issue
callQueuerecreated on every render – enqueued promises can be lost
const callQueue: QueuedCall[] = []lives insideuseSignIn, so each re-render allocates a fresh array.
Any proxy created in an earlier render closes over the old array; its queued calls will never be flushed onceclientbecomes available.Same concern was flagged earlier but the implementation is unchanged.
Recommended fix (sketch):
- const callQueue: QueuedCall[] = []; + const callQueueRef = useRef<QueuedCall[]>([]); ... - while (callQueue.length > 0) { - const queuedCall = callQueue.shift(); + while (callQueueRef.current.length > 0) { + const queuedCall = callQueueRef.current.shift();and push to
callQueueRef.current.
200-206:⚠️ Potential issueHook invocation inside
setTimeoutcallback still violates React rules
useClientContext()anduseIsomorphicClerkContext()are called inside the asynchronoussetTimeouthandler. Hooks must only be invoked unconditionally at the top level of a component or custom hook – never inside callbacks.The prior review comment highlighting this breach was not addressed.
Replace those hook calls with the captured
client/isomorphicClerkreferences or trigger a re-render via state instead.
245-249: 🛠️ Refactor suggestion
isLoadedis alwaystrueeven when the real client is absentReturning
isLoaded: truealongside proxy placeholders is misleading – consumers rely on this flag to know the resource is truly ready.
Earlier feedback pointed this out; behaviour is unchanged.Either defer
isLoadeduntilclientis available or rename the flag (e.g.isDeferred) and update downstream usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react/src/hooks/useSignIn.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
| const createProxy = <T>(target: 'signIn' | 'setActive'): T => { | ||
| const proxyTarget = target === 'signIn' ? { observable: () => ({}) } : {}; | ||
|
|
||
| return new Proxy( | ||
| proxyTarget, | ||
| { | ||
| get(_, prop) { | ||
| // Handle the observable property for the proxy as well | ||
| if (prop === 'observable') { |
There was a problem hiding this comment.
setActive proxy is not callable – will throw TypeError: xyz is not a function
When client is not ready the fallback proxy for setActive is created over a plain object, yet downstream code expects a function.
Invoking setActive() will therefore explode before the call ever reaches the queue.
Proposed minimal patch:
- const proxyTarget = target === 'signIn' ? { observable: () => ({}) } : {};
+ const proxyTarget =
+ target === 'signIn'
+ ? { observable: () => ({}) }
+ : (..._args: unknown[]) => undefined; // make proxyTarget callable
...
- get(_, prop) {
+ apply(_unused, _thisArg, args) {
+ return new Promise((resolve, reject) => {
+ callQueue.push({
+ target,
+ method: 'apply',
+ args,
+ resolve,
+ reject,
+ });
+ });
+ },
+ get(_, prop) {This turns the proxy into a callable function and routes direct invocations through the queue.
Also applies to: 185-199, 230-240
🤖 Prompt for AI Agents
In packages/react/src/hooks/useSignIn.ts around lines 168 to 176, the proxy
created for 'setActive' is a plain object but downstream code expects it to be
callable as a function, causing a TypeError when invoked. To fix this, change
the proxy target for 'setActive' from an object to a callable function (e.g., an
empty function) so that invoking setActive() does not throw an error and the
call can be routed through the queue. Apply the same fix to similar proxy
creations around lines 185-199 and 230-240.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/react/src/hooks/useSignIn.ts (5)
151-170:⚠️ Potential issue
callQueuerecreated on each render → promises silently lost
callQueueis defined inside the hook body, so every re-render allocates a fresh array.
Promises enqueued by proxies created during previous renders will never be flushed, leaving callers hanging.- const callQueue: QueuedCall[] = []; + // Persist across renders + const callQueueRef = useRef<QueuedCall[]>([]);Subsequently replace all
callQueuereferences withcallQueueRef.current.This issue was raised in an earlier review but is still present.
221-229:⚠️ Potential issueReact Hook still called inside
setTimeoutcallback
useClientContext()(anduseIsomorphicClerkContext()) are invoked inside thesetTimeoutcallback, continuing to violate the Rules-of-Hooks.Capture the references in the outer scope instead:
- setTimeout(() => { - // Re-check context when the queued call executes - const currentClient = useClientContext(); - const currentIsomorphicClerk = useIsomorphicClerkContext(); + const currentClient = client; + const currentIsomorphicClerk = isomorphicClerk; + setTimeout(() => { if (currentClient) { processQueue( currentClient.signIn, currentIsomorphicClerk.setActive, ); } }, 0);Unresolved from the previous review.
179-195:⚠️ Potential issue
setActiveproxy is not callable – will throwTypeError
proxyTargetis an object, yet downstream code treatssetActiveas a function.
Invoking it will explode before the call is queued.- const proxyTarget: any = {}; + const proxyTarget: any = + target === 'setActive' + ? (..._args: unknown[]) => undefined // make proxy callable + : {};Add an
applytrap to route direct calls through the queue if needed.Previously reported, still outstanding.
158-166: 🛠️ Refactor suggestionMissing method-existence check in
processQueueInvoking a non-existent method will throw synchronously. Validate before calling:
- const targetObj = target === 'setActive' ? setActive : signIn; - const result = (targetObj as any)[method](...args); + const targetObj = target === 'setActive' ? setActive : signIn; + if (typeof (targetObj as any)[method] !== 'function') { + reject(new Error(`Method ${method} does not exist on ${target}`)); + continue; + } + const result = (targetObj as any)[method](...args);Already suggested earlier; still missing.
256-260:⚠️ Potential issue
isLoadedis alwaystrueeven when only proxies are returnedDown-stream consumers rely on this flag to know when the real client is ready.
Returnfalseuntilclientis available, or rename the flag to avoid breaking the contract.Issue persists from previous review.
🧹 Nitpick comments (1)
packages/react/src/hooks/useSignIn.ts (1)
57-85: Remove verboseconsole.logstatements before shippingThe numerous debug logs leak internal state (e.g. store contents) and clutter production output.
Either guard with
process.env.NODE_ENV !== 'production'or drop them entirely.- // Debug logging - console.log('createStoreObservable debug:', { ... });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react/src/hooks/useSignIn.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/react/src/hooks/useSignIn.ts (1)
52-55: Function using hooks does not start withuse*– ESLint rule-of-hooks blind spotThe anonymous function returned by
createStoreObservablecallsuseStore, but its name doesn’t start with “use”.
ESLint will not validate correct usage, increasing risk of accidental conditional invocation.Rename the returned function to
function useSignInStore() { … }and expose it accordingly.[ suggest_essential_refactor ]
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/react/src/hooks/useSignIn.ts (4)
173-190:⚠️ Potential issue
callQueuerecreated on every render → queued promises are lost
callQueueis a plain array defined inside the hook body. Each re-render allocates a new array, so any promises stored before the first client hydration are silently dropped.- const callQueue: QueuedCall[] = []; + // Persist across renders + const callQueueRef = useRef<QueuedCall[]>([]);…and update all reads/writes to
callQueueRef.current.Also move
processQueueinvocation into auseEffectso it runs wheneverclientbecomes available.
175-186: 🛠️ Refactor suggestionValidate method before invocation & surface context on error
Invoking
(targetObj as any)[method]without existence checks can throw or silently fail on typos. Add a guard:- const result = (targetObj as any)[method](...args); + const fn = (targetObj as any)[method]; + if (typeof fn !== 'function') { + reject(new Error(`Method ${method} does not exist on ${target}`)); + continue; + } + const result = fn(...args);
191-199:⚠️ Potential issue
isLoadedistruewhen resources are still proxies → breaks contractWhen
clientis falsy we still returnisLoaded: truetogether with proxy placeholders that may never resolve (e.g. SSR failure).
Either:
- expose
isLoaded: falseuntil the real client is present, or- rename the flag (e.g.
isDeferred) and update docs/consumers.Failing to adjust will cause subtle runtime errors in downstream code relying on the original semantics.
201-216:⚠️ Potential issue
setActiveproxy is not callable –TypeError: proxy is not a function
createProxy('setActive')returns an object proxy, but callers expect
setActive(...)to be invokable. Wrap a dummy function instead and forward
applyto the queue:- const proxyTarget: any = {}; + // Make the proxy callable + const proxyTarget: any = + target === 'setActive' ? function noop() {} : {}; ... - get(_, prop) { + // Intercept both property access *and* direct calls + apply(_unused, _thisArg, args) { + return new Promise((resolve, reject) => { + callQueueRef.current.push({ + target, + method: 'apply', + args, + resolve, + reject, + }); + }); + }, + get(_, prop) {Without this fix any early
setActive()call explodes before reaching the queue.Also applies to: 229-241
🧹 Nitpick comments (1)
packages/react/src/hooks/useSignIn.ts (1)
127-142: Hook hiding from ESLint – expose a properuse*API
observablereturns a function that contains hooks (useStore) but is invoked
viasignIn.observable(). ESLint’s rule-of-hooks cannot detect misuse (e.g.
conditional calls). Consider publishing a real hook instead:// Outside the resource export function useSignInStore(signIn: SignInResource) { return createStoreObservable(signIn)(); }This preserves type safety and linter protection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react/src/hooks/useSignIn.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/react/src/hooks/useSignIn.ts (4)
72-85: Un-guarded verboseconsole.logstill presentAn earlier review already requested gating or removing these logs to avoid leaking internals and bloating production bundles.
They are still unconditional.- console.log('createStoreObservable debug:', { + if (process.env.NODE_ENV !== 'production') { + /* eslint-disable no-console */ + console.debug('[Clerk] useSignIn observable:', { /* … */ - }); + }); + /* eslint-enable no-console */ + }
186-204:callQueuerecreated on every render → promises are lost
const callQueue: QueuedCall[] = []is scoped inside the hook body, so each
render allocates a new array. Calls queued by proxies created in previous
renders will never be flushed, leaving callers hanging.Past feedback already covered this; consider hoisting it into
useRef(or a
module-level singleton) and processing it inside auseEffect.
214-244:⚠️ Potential issue
setActiveproxy is not callable – invocations will throwTypeError
proxyTargetis a plain object and noapplytrap is defined, so
setActiveProxy()explodes before it even hits the queue.
This was flagged previously and is still unresolved.- const proxyTarget: any = {}; + // Make the proxy target callable so `setActive()` does not crash + const proxyTarget: any = + target === 'setActive' + ? (..._args: unknown[]) => undefined + : {};Also add an
applytrap that funnels direct calls through the queue:+ apply(_unused, _thisArg, args) { + return new Promise((resolve, reject) => { + callQueue.push({ + target, + method: 'apply', + args, + resolve, + reject, + }); + }); + },
286-289:⚠️ Potential issue
isLoadedreportstruewhile only proxies are returnedDown-stream code relies on
isLoadedto know when the realsignIn/
setActiveare usable. Returningtruewith placeholders breaks that contract
and can surface subtle runtime errors.This has been highlighted in previous reviews but remains. Either:
- Keep
isLoadedfalseuntilclientis ready, or- Rename the flag (e.g.
isDeferred) and update consumers.
🧹 Nitpick comments (1)
packages/react/src/hooks/useSignIn.ts (1)
1-1: Style nit: missing whitespace after comma in import listThe first import statement lacks a space after the comma, which is likely to break the repo-wide lint rule (
comma-spacing) and create unnecessary noise in CI.-import { ClerkInstanceContext,useClientContext } from '@clerk/shared/react'; +import { ClerkInstanceContext, useClientContext } from '@clerk/shared/react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react/src/hooks/useSignIn.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/react/src/hooks/useSignIn.ts (2)
159-176:⚠️ Potential issueQueued promises are lost on every render – hoist the queue into a ref
callQueuelives in render scope, so any re-render (e.g.clientupdate) swaps it for a new array.
Promises enqueued by proxy calls in earlier renders will never be flushed, causing silent hangs.Move the queue to
useRefand flush it fromuseEffect.- const callQueue: QueuedCall[] = []; + const callQueueRef = useRef<QueuedCall[]>([]); @@ - const processQueue = (signIn: SignInResource, setActive: SetActive) => { - while (callQueue.length > 0) { - const queuedCall = callQueue.shift(); + const processQueue = (signIn: SignInResource, setActive: SetActive) => { + while (callQueueRef.current.length > 0) { + const queuedCall = callQueueRef.current.shift(); @@ - callQueue.push({ + callQueueRef.current.push({ @@ - if (client) { - processQueue(client.signIn, isomorphicClerk.setActive); + useEffect(() => { + if (client) { + processQueue(client.signIn, isomorphicClerk.setActive); + } + }, [client]);This guarantees queued calls survive until the real client is ready.
187-260:⚠️ Potential issue
setActiveproxy isn’t callable – runtime will throwTypeError: … is not a functionThe proxy’s target is a plain object, so invoking
setActive()explodes before the queue handler runs.
Swap to a callable target (empty function) and implement anapplytrap.- const proxyTarget: any = {}; + // function target so the proxy itself is callable + const proxyTarget: any = + target === 'setActive' + ? (..._args: unknown[]) => undefined + : {}; @@ - get(_, prop) { + apply(_unused, _thisArg, args) { + return new Promise((resolve, reject) => { + callQueueRef.current.push({ + target, + method: 'apply', + args, + resolve, + reject, + }); + }); + }, + get(_, prop) {This mirrors the Vue implementation and prevents hard crashes.
🧹 Nitpick comments (3)
packages/react/src/hooks/useSignIn.ts (3)
1-1: Add spacing after comma for readability and eslint consistency
import { ClerkInstanceContext,useClientContext }misses a space after the comma – most style-guides (incl. the repo’s eslint config) requireFoo, Bar.
Purely cosmetic, but keeps diffs noise-free.-import { ClerkInstanceContext,useClientContext } from '@clerk/shared/react'; +import { ClerkInstanceContext, useClientContext } from '@clerk/shared/react';
51-56:subscribesignature is incompatible with Zustand – add the listener param
useStorewill callstore.subscribe(listener, …), but the fallback store’ssubscribeaccepts zero args.
While JS allows extra args, TypeScript’s structural typing flags this as an error and it breaks intellisense.
Add the param (and ignore it) to remain API-compatible.- subscribe: () => () => {}, // Return unsubscribe function + subscribe: (_listener: () => void) => () => {}, // noop unsubscribe
85-92: Debugconsole.logcalls leak noise to production bundlesUn-guarded logs inside a hook fire on every render and ship to prod builds, hurting perf and exposing internals.
Wrap them in a dev-only guard or remove entirely.- console.log('No real store found, returning empty object'); + if (process.env.NODE_ENV !== 'production') { + /* eslint-disable no-console */ + console.debug('[Clerk] useSignIn: no store – returning empty object'); + /* eslint-enable no-console */ + } @@ - console.log('Returning store state:', storeState); + if (process.env.NODE_ENV !== 'production') { + /* eslint-disable no-console */ + console.debug('[Clerk] useSignIn store state', storeState); + /* eslint-enable no-console */ + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react/src/hooks/useSignIn.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/react/src/hooks/useSignIn.ts (6)
packages/types/src/signIn.ts (1)
SignInResource(76-127)packages/clerk-js/src/core/resources/Base.ts (1)
store(63-65)packages/vue/src/composables/useSignIn.ts (1)
useSignIn(47-128)packages/shared/src/react/contexts.tsx (3)
ClerkInstanceContext(96-96)useClientContext(87-87)useAssertWrappedByClerkProvider(98-98)packages/react/src/isomorphicClerk.ts (1)
client(633-640)packages/types/src/clerk.ts (1)
SetActive(1073-1073)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/react/src/hooks/useSignIn.ts (1)
262-266:isLoaded: truewhile returning proxies misleads consumers
isLoadedtraditionally signals that real Clerk resources are ready.
Here it is alwaystrue, even whensignIn/setActiveare placeholders that may never resolve (SSR, network error).Either:
- Keep
isLoaded = client !== undefined, or- Rename the flag (e.g.
isDeferred) and update docs + downstream usage.Failing to align the semantic contract risks subtle runtime bugs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/react/src/hooks/useSignIn.ts (2)
107-157:⚠️ Potential issueProxy for
setActiveis not callable & generic constraint is redundant
createProxybuilds the fallback forsetActiveover{}; invoking it (setActive()) will throwTypeError: ... is not a function. Provide a callable target and anapplytrap that enqueues direct invocations.
T extends anyis the exact lint error reported – the constraint is useless; just useT.- const createProxy = useCallback(<T extends any>(target: 'signIn' | 'setActive'): T => { - const proxyTarget: any = {}; + const createProxy = useCallback<<T>(target: 'signIn' | 'setActive'): T => { + // make setActive callable, keep signIn an object + const proxyTarget: any = + target === 'setActive' + ? (..._args: unknown[]) => undefined + : {}; ... - { + { + // Allow direct invocation of the proxy (for setActive) + apply(_unused, _thisArg, args) { + return new Promise((resolve, reject) => { + callQueueRef.current.push({ + target, + method: 'apply', + args, + resolve, + reject, + }); + }); + }, get(_, prop) {This prevents runtime crashes and satisfies the static-analysis complaint in one go.
🧰 Tools
🪛 Biome (1.9.4)
[error] 107-107: Constraining a type parameter to any or unknown is useless.
All types are subtypes of any and unknown.
Safe fix: Remove the constraint.(lint/complexity/noUselessTypeConstraint)
🪛 ESLint
[error] 107-107: Constraining the generic type
Ttoanydoes nothing and is unnecessary.(@typescript-eslint/no-unnecessary-type-constraint)
89-106:⚠️ Potential issueQueued calls are lost on every re-render – hoist the queue into a stable ref
callQueueis re-created on each hook invocation, so promises enqueued by proxies returned during the first render will never be flushed onceclientbecomes truthy in a later render.
This reproduces the exact bug already pointed out in earlier review comments.- const callQueue: QueuedCall[] = []; + // Persist across renders + const callQueueRef = useRef<QueuedCall[]>([]);and everywhere else:
- const queuedCall = callQueue.shift(); + const queuedCall = callQueueRef.current.shift(); ... - callQueue.push({ + callQueueRef.current.push({Finally, trigger
processQueuefrom auseEffectto guarantee it runs whenclientappears:- if (client) { - processQueue(client.signIn, isomorphicClerk.setActive); - } + useEffect(() => { + if (client) { + processQueue(client.signIn, isomorphicClerk.setActive); + } + }, [client, processQueue]);Without this, any deferred calls made before Clerk hydrates will hang forever.
🧹 Nitpick comments (1)
packages/react/src/hooks/useSignIn.ts (1)
4-4: Minor nit: missing space in imports-import { useCallback,useContext, useMemo } from 'react'; +import { useCallback, useContext, useMemo } from 'react';Readability only; no functional impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react/src/hooks/useSignIn.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/react/src/hooks/useSignIn.ts
[error] 107-107: Constraining a type parameter to any or unknown is useless.
All types are subtypes of any and unknown.
Safe fix: Remove the constraint.
(lint/complexity/noUselessTypeConstraint)
🪛 ESLint
packages/react/src/hooks/useSignIn.ts
[error] 107-107: Constraining the generic type T to any does nothing and is unnecessary.
(@typescript-eslint/no-unnecessary-type-constraint)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/react/src/hooks/useSignIn.ts (2)
181-186:isLoaded: trueeven when only proxies are returned – breaks contractDown-stream code historically relied on
isLoadedbefore callingsignIn/setActive. Returningtruewhile handing out dummy proxies is misleading and breaks semantic guarantees.Please either:
- Keep
isLoaded === falseuntil a realclientexists, or- Rename the flag (e.g.
isDeferred) and update all consumer types/docs accordingly.Failing to do so risks subtle production errors when Clerk never hydrates (SSR, network fault, etc.).
60-63: Skipping the provider assertion hides configuration errors
useAssertWrappedByClerkProvideris only invoked whenclerkInstanceContextis truthy, effectively disabling the runtime guard when the provider is missing (the very case it should detect).
Was this relaxation intentional? If not, revert to unconditional assertion or add an explicit comment explaining the new expectation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/react/src/hooks/useSignIn.ts (2)
175-185:isLoadedis alwaystrueeven when real client is absentThe hook advertises readiness while still returning proxies that may never
resolve (SSR, network error, etc.), breaking the historical contract that
isLoadedguarantees a usable resource.Either:
- Keep
isLoaded = falseuntilclientis truthy, or- Rename the flag (
isDeferred?) and update all consumers/docs.This concern has been raised in previous rounds but remains unaddressed.
90-107: 🛠️ Refactor suggestionQueue is re-created per render – risk of losing calls
callQueueis declared inside the hook body.
A re-renderallocates a fresh array, butcreateProxy&processQueue
instances from the first render keep a reference to the old queue; any
promises enqueued by proxies created after the first render are pushed onto
a queue thatprocessQueuewill never drain.Stabilise the queue with
useRef(or module scope):- const callQueue: QueuedCall[] = []; + const callQueueRef = useRef<QueuedCall[]>([]); ... - while (callQueue.length > 0) { - const queuedCall = callQueue.shift(); + while (callQueueRef.current.length > 0) { + const queuedCall = callQueueRef.current.shift(); ... - callQueue.push({ + callQueueRef.current.push({And run
processQueuefrom auseEffectwhenclientbecomes available so the
queue is always flushed.This was flagged in earlier reviews but is still unresolved.
🧹 Nitpick comments (1)
packages/react/src/hooks/useSignIn.ts (1)
4-4: Minor formatting nit – missing space after comma
import { useCallback,useContext, useMemo } from 'react';Add a space after
useCallbackfor consistency with the rest of the imports.-import { useCallback,useContext, useMemo } from 'react'; +import { useCallback, useContext, useMemo } from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react/src/hooks/useSignIn.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
| const createProxy = useCallback(<T>(target: 'signIn' | 'setActive'): T => { | ||
| const proxyTarget: any = {}; | ||
|
|
||
| return new Proxy( | ||
| proxyTarget, | ||
| { | ||
| get(_, prop) { | ||
| // Prevent React from treating this proxy as a Promise by returning undefined for 'then' | ||
| if (prop === 'then') { | ||
| return undefined; | ||
| } | ||
|
|
||
| // Handle Symbol properties and other non-method properties | ||
| if (typeof prop === 'symbol' || typeof prop !== 'string') { | ||
| return undefined; | ||
| } | ||
|
|
||
| // For observableState property, return undefined in proxy mode | ||
| if (prop === 'observableState' && target === 'signIn') { | ||
| return undefined; | ||
| } | ||
|
|
||
| return (...args: any[]) => { | ||
| return new Promise((resolve, reject) => { | ||
| callQueue.push({ | ||
| target, | ||
| method: String(prop), | ||
| args, | ||
| resolve, | ||
| reject, | ||
| }); | ||
| }); | ||
| }; | ||
| }, | ||
| has(_, prop) { | ||
| // Return false for 'then' to prevent Promise-like behavior | ||
| if (prop === 'then') { | ||
| return false; | ||
| } | ||
| // Return true for all other properties to indicate they exist on the proxy | ||
| return true; | ||
| }, | ||
| ownKeys(_) { | ||
| return Object.getOwnPropertyNames(proxyTarget); | ||
| }, | ||
| getOwnPropertyDescriptor(_, prop) { | ||
| return Object.getOwnPropertyDescriptor(proxyTarget, prop); | ||
| } | ||
| }, | ||
| ) as T; | ||
| }, []); |
There was a problem hiding this comment.
setActive proxy is not callable – will throw at runtime
createProxy always uses a plain object ({}) as proxyTarget.
When the target is 'setActive', downstream code expects a function and will do:
const { setActive } = useSignIn();
await setActive('someSessionId');A proxy over a non-callable object cannot be invoked and will raise
TypeError: setActive is not a function.
Fix: make the proxy target callable and implement an apply trap so direct
invocations are routed through the queue, in line with how property access is
handled for signIn methods.
const createProxy = useCallback(<T>(target: 'signIn' | 'setActive'): T => {
- const proxyTarget: any = {};
+ // Function targets stay callable; object targets keep method semantics
+ const proxyTarget: any =
+ target === 'setActive' ? (..._args: unknown[]) => undefined : {};
return new Proxy(
proxyTarget,
{
+ /* Proxy invoked like a function → enqueue */
+ apply(_unused, _thisArg, args) {
+ return new Promise((resolve, reject) => {
+ callQueue.push({
+ target,
+ // Special marker for direct invocation
+ method: 'apply',
+ args,
+ resolve,
+ reject,
+ });
+ });
+ },This keeps setActive usable before Clerk is loaded and prevents hard crashes.
🤖 Prompt for AI Agents
In packages/react/src/hooks/useSignIn.ts between lines 108 and 158, the
createProxy function uses a plain object as the proxy target for both 'signIn'
and 'setActive', but 'setActive' is expected to be callable. To fix this, change
the proxy target to a callable function when target is 'setActive' and add an
apply trap to the proxy handler that routes direct function calls through the
callQueue, similar to how property accesses are handled. This will prevent
runtime TypeErrors when invoking setActive before Clerk is loaded.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/react/src/hooks/useSignIn.ts (3)
175-185:isLoaded: truewhile resources are still proxies misleads consumers
(Re-raising previous feedback)The hook returns
isLoaded: trueeven whenclientis absent and bothsignIn&setActiveare dummy proxies that might never resolve (e.g. SSR, network error).
Down-stream code that correctly waits forisLoadedwill falsely assume the resource is ready.Either:
- Keep
isLoaded=falseuntilclientis truthy, or- Rename the flag (e.g.
isDeferred) and update docs/tests.Failing to fix will cause subtle runtime errors in existing apps.
91-108:⚠️ Potential issue
callQueuerecreated on every render → lost promises
(Re-raising previous feedback)
const callQueue: QueuedCall[] = []lives inside the hook’s render scope, so each re-render allocates a fresh array.
Proxies created during earlier renders push into an obsolete queue that is never flushed, leaving callers hanging indefinitely.See prior comment “Queue is recreated on every render → pending calls may be lost”.
Please hoist the queue into auseRefand flush it in an effect:-import { useCallback, useContext, useMemo } from 'react'; +import { useCallback, useContext, useMemo, useRef, useEffect } from 'react'; - const callQueue: QueuedCall[] = []; + const callQueueRef = useRef<QueuedCall[]>([]); const processQueue = useCallback((signIn: SignInResource, setActive: SetActive) => { - while (callQueue.length > 0) { - const queuedCall = callQueue.shift(); + while (callQueueRef.current.length > 0) { + const queuedCall = callQueueRef.current.shift(); ... }, []); // flush once the client is available - if (client) { - processQueue(client.signIn, isomorphicClerk.setActive); - } + useEffect(() => { + if (client) { + processQueue(client.signIn, isomorphicClerk.setActive); + } + }, [client, processQueue, isomorphicClerk?.setActive]);
109-159:⚠️ Potential issue
setActiveproxy is not callable – will throwTypeError: xxx is not a function
(Re-raising previous feedback)When
clientis undefined the hook returns a proxy forsetActive, but the proxy target is a plain object, not a function.
Any consumer invokingsetActive()before hydration will crash.Patch:
- const proxyTarget: any = {}; + // keep function semantics for 'setActive', object semantics for 'signIn' + const proxyTarget: any = target === 'setActive' ? () => undefined : {}; + // Allow direct invocation of the proxy (apply trap) + apply(_unused, _thisArg, args) { + return new Promise((resolve, reject) => { + callQueueRef.current.push({ + target, + method: 'apply', + args, + resolve, + reject, + }); + }); + },This keeps the proxy callable and routes the call through the queue.
🧹 Nitpick comments (1)
packages/react/src/hooks/useSignIn.ts (1)
4-4: Missing space in import list – triggers eslint/prettier noise
useCallback,useContextlacks a space after the comma.
While harmless at runtime, it breaks the project’s formatting conventions and will be auto-fixed by any formatter run.-import { useCallback,useContext, useMemo } from 'react'; +import { useCallback, useContext, useMemo } from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react/src/hooks/useSignIn.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
| getState: () => FALLBACK_STATE, | ||
| getInitialState: () => FALLBACK_STATE, | ||
| subscribe: () => () => {}, // Return unsubscribe function | ||
| setState: () => {}, |
There was a problem hiding this comment.
subscribe signature incompatible with Zustand → runtime/type errors
useStore expects the store’s subscribe method to accept a listener parameter ((listener, selector?, equalityFn?)), but the fallback stub is declared with no parameters.
Type-checking will fail and, at runtime, Zustand will still call the function with arguments that will be silently ignored, making the fallback store impossible to observe.
- subscribe: () => () => {}, // Return unsubscribe function
+ subscribe: (_listener: unknown) => () => {}, // accept listener to satisfy Zustand📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getState: () => FALLBACK_STATE, | |
| getInitialState: () => FALLBACK_STATE, | |
| subscribe: () => () => {}, // Return unsubscribe function | |
| setState: () => {}, | |
| getState: () => FALLBACK_STATE, | |
| getInitialState: () => FALLBACK_STATE, | |
| subscribe: (_listener: unknown) => () => {}, // accept listener to satisfy Zustand | |
| setState: () => {}, |
🤖 Prompt for AI Agents
In packages/react/src/hooks/useSignIn.ts around lines 48 to 51, the fallback
store's subscribe method is declared without parameters, which is incompatible
with Zustand's expected signature that requires a listener and optional selector
and equality function parameters. Update the subscribe method to accept these
parameters and return an unsubscribe function, ensuring it matches Zustand's
signature to avoid type errors and enable proper observation of the fallback
store.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/clerk-js/sandbox/app.ts (1)
268-279: Repeated override ofconsole.error– keep previous feedback
The global hijack ofconsole.erroris still present. This has already been flagged as risky because it pollutes global state and can break other logging / error-reporting logic. Please replace with a scoped handler (e.g. a local wrapper or try/catch) as suggested earlier.
🧹 Nitpick comments (2)
packages/clerk-js/sandbox/app.ts (2)
868-896: Busy-wait polling for Clerk readiness – switch to event/promise API
ThewaitForClerkhelper recursivelysetTimeouts every 100 ms. This is CPU-friendly but still unnecessary; Clerk exposesClerk.addListener('loaded', …)and returns a promise fromClerk.load()you can await.Refactor suggestion:
- await waitForClerk(); + await Clerk.load(); // resolves when both library and client are readyThis removes ~30 LOC, avoids custom polling, and makes failure-handling explicit.
1058-1066:cleanupnever invoked → potential subscription leak
You create(element as any)._cleanup = cleanup, but no code calls it on route change or page unload. Navigating between routes repeatedly mounts new subscriptions and leaked timers.Consider:
window.addEventListener('beforeunload', cleanup); document.addEventListener('clerk:sandbox:route-change', cleanup); // custom event you dispatch before mounting new routeor store the unsubscribe in a higher-level router so each mount’s teardown is guaranteed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/clerk-js/sandbox/app.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
| // Update status to show we're creating SignIn | ||
| statusContainer.innerHTML = ` | ||
| <div class="text-blue-500"> | ||
| <strong>Status:</strong> Creating SignIn with identifier: ${emailInput.value} | ||
| </div> | ||
| `; |
There was a problem hiding this comment.
Unsanitised interpolation → XSS vector
emailInput.value and error.message are injected directly into innerHTML. An attacker (or an accidentally-malformed email) could smuggle HTML/JS and execute arbitrary code.
- statusContainer.innerHTML = `
- <div class="text-blue-500">
- <strong>Status:</strong> Creating SignIn with identifier: ${emailInput.value}
- </div>
- `;
+ statusContainer.textContent =
+ `Status: Creating SignIn with identifier: ${emailInput.value}`;Likewise for the error block:
- <div class="text-sm">${errorMessage}</div>
+ <div class="text-sm" id="errorText"></div>
document.getElementById('errorText')!.textContent = errorMessage;If preserving rich markup is required, run the value through a trusted sanitiser (e.g. DOMPurify) before assignment.
Also applies to: 1038-1047
🤖 Prompt for AI Agents
In packages/clerk-js/sandbox/app.ts around lines 941 to 946, the code directly
injects user input (emailInput.value) into innerHTML, creating an XSS
vulnerability. To fix this, sanitize the emailInput.value using a trusted
sanitizer like DOMPurify before assigning it to innerHTML. Apply the same
sanitization approach to any other user inputs or error messages inserted as
HTML, including the block at lines 1038-1047.
Description
Demo is here
Observable Store Architecture - Key Benefits
The SignIn Observable Store Demo showcases a new architecture pattern where resources expose a
.storeproperty containing a vanilla JS Zustand store. This enables reactive state management across any framework while maintaining backward compatibility.Framework-Agnostic Reactive Integration
The
.storeproperty is a vanilla JS Zustand store that any framework can integrate with for reactive updates:Benefits:
Non-Breaking Progressive Enhancement
The new
.storeproperty enables gradual adoption without version coupling:Benefits:
Internal Architecture Observability
The observable store powers resource internals regardless of UI usage:
Benefits:
Architecture Pattern
Impact
This pattern enables:
useSignIn hook without the need for
isLoadedReturning a lazy loaded proxy object from
useSignInuntil clerk-js is loadedSummary by CodeRabbit
New Features
Improvements
Chores