feat(shared,nextjs,react): Introduce useOAuthConsent hook#8286
feat(shared,nextjs,react): Introduce useOAuthConsent hook#8286wobsoriano merged 8 commits intomainfrom
useOAuthConsent hook#8286Conversation
🦋 Changeset detectedLatest commit: 71ad9f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 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.
|
|
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 a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/react/hooks/useOAuthConsent.tsx`:
- Around line 53-59: The hook accesses window.location.search unsafely in the
useOAuthConsent hook: update the initial state for searchSnapshot and the
useLayoutEffect so they only read window.location.search when window and
window.location exist (e.g. check typeof window !== 'undefined' &&
window.location) or use optional chaining (window?.location?.search) before
calling setSearchSnapshot; modify references to searchSnapshot/setSearchSnapshot
and the useLayoutEffect callback accordingly to avoid reads when location is
undefined (this prevents crashes in React Native/Expo).
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b1f703c3-9260-48ce-b420-5e1b7b372cea
📒 Files selected for processing (12)
.changeset/oauth-consent-use-hook.mdpackages/expo/src/hooks/index.tspackages/nextjs/src/client-boundary/hooks.tspackages/nextjs/src/index.tspackages/react/src/hooks/index.tspackages/shared/src/react/hooks/__tests__/useOAuthConsent.shared.spec.tspackages/shared/src/react/hooks/__tests__/useOAuthConsent.spec.tsxpackages/shared/src/react/hooks/index.tspackages/shared/src/react/hooks/useOAuthConsent.shared.tspackages/shared/src/react/hooks/useOAuthConsent.tsxpackages/shared/src/react/hooks/useOAuthConsent.types.tspackages/shared/src/react/stable-keys.ts
| * OAuth consent screen metadata from Clerk, or `undefined` before the first successful fetch. | ||
| * Additional fields (e.g. submission helpers) may be added in the future without renaming this hook. | ||
| */ | ||
| data: OAuthConsentInfo | undefined; |
There was a problem hiding this comment.
Might I regret typing this as OAuthConsentInfo? If I need to add a member for e.g. post consent response endpoint? or a method?
There was a problem hiding this comment.
Good instinct. Let's keep data as the raw FAPI response and put the submission helpers (even the submissionEndpoint property we talked about) at root level alongside data, similar to how other hooks return additional properties.
That way data stays a clean mirror of the API response, and hook utilities live at the top level
| const ORGANIZATION_CREATION_DEFAULTS_KEY = 'organizationCreationDefaults'; | ||
|
|
||
| // Keys for `useOAuthConsent` (consent metadata GET; keep distinct from future submit/mutation keys) | ||
| const OAUTH_CONSENT_INFO_KEY = 'oauthConsentInfo'; |
There was a problem hiding this comment.
what are these keys for? Do I need to think about future members?
There was a problem hiding this comment.
These are tanstack query cache keys. Every query-based hook needs a unique stable key for caching/invalidation.
Tbh we dont really need caching/invalidation here as the page gets visited only once by user, but since we are using our tanstack query wrapper internally, we need a cache key
| /** | ||
| * Whether any request is still in flight, including background updates. | ||
| */ | ||
| isFetching: boolean; |
There was a problem hiding this comment.
Do we need both isFetching and isLoading?
There was a problem hiding this comment.
We need the isLoading here to check if the consent info is being fetched initially or not.
The isFetching is useless here as the page gets visited only once (this is for backgroudn fetches) but to follow existing hooks, we keep it for consistency
| * | ||
| * @default true | ||
| */ | ||
| keepPreviousData?: boolean; |
There was a problem hiding this comment.
To be consistent with other hooks, yes
| typeof window !== 'undefined' ? window.location.search : '', | ||
| ); | ||
|
|
||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
why not use useEffect here? 🤔
There was a problem hiding this comment.
Agreed, useEffect would be fine here. But I think we can simplify further: the URL doesn't change after load (consent pages are full-navigation redirects), so we can drop the state + effect entirely and just read it once in a useMemo.
Also coderabbit flagged this for Expo users, I'll clean this up
| if (!getConsentInfo) { | ||
| return Promise.reject(new Error('OAuth consent is not available in this Clerk instance')); | ||
| } | ||
| const p: GetOAuthConsentInfoParams = scope !== undefined ? { oauthClientId, scope } : { oauthClientId }; |
There was a problem hiding this comment.
note that scope is optional
| }); | ||
|
|
||
| return { | ||
| data: query.data, |
There was a problem hiding this comment.
let's change data to consentInfo
There was a problem hiding this comment.
After doing another review, I'd say we should keep data here. Every other useClerkQuery-based hook returns data, not a named field.
The hook name already tells us what data is here, which is the consent info, and consumers can always destructure as const { data: consentInfo } = useOAuthConsent() if they want a named variable.
|
!snapshot |
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@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/vue
commit: |
| }); | ||
|
|
||
| return { | ||
| data: query.data, |
There was a problem hiding this comment.
After doing another review, I'd say we should keep data here. Every other useClerkQuery-based hook returns data, not a named field.
The hook name already tells us what data is here, which is the consent info, and consumers can always destructure as const { data: consentInfo } = useOAuthConsent() if they want a named variable.
| const ORGANIZATION_CREATION_DEFAULTS_KEY = 'organizationCreationDefaults'; | ||
|
|
||
| // Keys for `useOAuthConsent` (consent metadata GET; keep distinct from future submit/mutation keys) | ||
| const OAUTH_CONSENT_INFO_KEY = 'oauthConsentInfo'; |
There was a problem hiding this comment.
These are tanstack query cache keys. Every query-based hook needs a unique stable key for caching/invalidation.
Tbh we dont really need caching/invalidation here as the page gets visited only once by user, but since we are using our tanstack query wrapper internally, we need a cache key
| typeof window !== 'undefined' ? window.location.search : '', | ||
| ); | ||
|
|
||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
Agreed, useEffect would be fine here. But I think we can simplify further: the URL doesn't change after load (consent pages are full-navigation redirects), so we can drop the state + effect entirely and just read it once in a useMemo.
Also coderabbit flagged this for Expo users, I'll clean this up
| /** | ||
| * Whether any request is still in flight, including background updates. | ||
| */ | ||
| isFetching: boolean; |
There was a problem hiding this comment.
We need the isLoading here to check if the consent info is being fetched initially or not.
The isFetching is useless here as the page gets visited only once (this is for backgroudn fetches) but to follow existing hooks, we keep it for consistency
| * OAuth consent screen metadata from Clerk, or `undefined` before the first successful fetch. | ||
| * Additional fields (e.g. submission helpers) may be added in the future without renaming this hook. | ||
| */ | ||
| data: OAuthConsentInfo | undefined; |
There was a problem hiding this comment.
Good instinct. Let's keep data as the raw FAPI response and put the submission helpers (even the submissionEndpoint property we talked about) at root level alongside data, similar to how other hooks return additional properties.
That way data stays a clean mirror of the API response, and hook utilities live at the top level
| * | ||
| * @default true | ||
| */ | ||
| keepPreviousData?: boolean; |
There was a problem hiding this comment.
To be consistent with other hooks, yes
useOAuthConsent hookuseOAuthConsent hook
useOAuthConsent hookuseOAuthConsent hook
- Simplify search snapshot logic (remove useState + useLayoutEffect) - Extract fetchConsentInfo into standalone function - Gate placeholderData on queryEnabled - Mark hook and types as @internal - Export from @clerk/react/internal and @clerk/nextjs/internal - Update export snapshots and changeset
8393e6f to
844917f
Compare
Description
This PR introduces an internal (for now)
useOAuthConsent()hook for fetching OAuth consent screen metadata for the signed-in user.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change