- 
                Notifications
    You must be signed in to change notification settings 
- Fork 403
fix(clerk-js): Display legal consent only based on missing fields #7101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| 🦋 Changeset detectedLatest commit: 65d5ddc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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. 
 | 
| WalkthroughDetects when only the  Changes
 Sequence Diagram(s)sequenceDiagram
    participant Start as SignUpStart
    participant Form as SignUpForm
    participant Fixture as Fixture/Test
    rect #E6F7FF
      Note right of Start: Compute props from session state
      Start->>Start: compute onlyLegalAcceptedMissing = useMemo(...)
    end
    Start->>Form: render SignUpForm(props..., onlyLegalAcceptedMissing)
    Form->>Form: conditional render (uses onlyLegalAcceptedMissing)
    Note right of Form: shows legal consent only when appropriate
    Fixture->>Start: supplies fixture (missing fields, unverified_fields)
    Fixture->>Form: asserts rendered output in tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 
 Poem
 Pre-merge checks and finishing touches❌ Failed checks (1 warning)
 ✅ Passed checks (2 passed)
 ✨ Finishing touches🧪 Generate unit tests (beta)
 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources: 
 
 📒 Files selected for processing (1)
 🧰 Additional context used📓 Path-based instructions (1).changeset/**📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc) 
 Files: 
 ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
 🔇 Additional comments (1)
 Comment  | 
| const onlyLegalAcceptedMissing = useMemo( | ||
| () => | ||
| signUp.missingFields && | ||
| signUp.missingFields.length === 1 && | ||
| signUp.missingFields[0] === 'legal_accepted' && | ||
| signUp.unverifiedFields && | ||
| signUp.unverifiedFields.length === 0, | ||
| [signUp.missingFields, signUp.unverifiedFields], | ||
| ); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already being done on SignUpContinue, but wasn't handled on the root sign-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of moving this check within SignUpForm so we don't repeat the same logic in both pages, however it seems that there was a reason why signUp isn't passed as a prop to it
My assumption is that SignUpForm is for form presentation concerns only and each screen would handle the logic around the sign up object
| @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:  | 
Description
We caught a bug on IdP-Initiated flows, with legal consent enabled, where FAPI would redirect to
/sign-upbut the form still gets shown with password even tho it's not required for enterprise SSOFor those cases, since legal consent is the only missing field and there aren't unverified fields, it should be the only one displayed.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Bug Fixes
Tests