-
Notifications
You must be signed in to change notification settings - Fork 403
feat(clerk-js,clerk-react,types): Navigate to next task #5377
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
Conversation
🦋 Changeset detectedLatest commit: f8b7a53 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 Git ↗︎
|
f644710 to
d63a9f5
Compare
d63a9f5 to
ccc0bd2
Compare
ccc0bd2 to
cdca177
Compare
280fbf3 to
714a946
Compare
714a946 to
54f5b52
Compare
54f5b52 to
76043f7
Compare
76043f7 to
b0f8a4d
Compare
7f33395 to
26d79ae
Compare
86d0dbc to
ad3a5d3
Compare
| status: 'pending', | ||
| user: {}, | ||
| tasks: [{ key: 'org' }], | ||
| currentTask: { key: 'org', __internal_getUrl: () => 'https://foocorp.com/add-organization' }, |
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.
For posterity: I'm always just a little nervous of including random routable URLs in tests.
Truly unlikely security threat potential: If we actually GET this URL, and someone knows we're getting it, they could register the domain and have it serve some exploit that gets us to leak our CI secrets.
I'm not even proposing you change this - it's the pattern laid down elsewhere in the test - but when in doubt, I like example.com or cnames of it. It's not routable. https://foocorp.example.com/add-organization is safer in my book.
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.
Makes sense! Also just to note that our tests only run if the triggering actor (owner of the PR) has the GitHub org permissions to access secrets, otherwise it'll fail
For URLs like those that are used to trigger navigation rather than HTTP requests, then I think it's fine!
| <SessionTask task='org' /> | ||
| <SessionTask | ||
| task='org' | ||
| redirectUrlComplete={signInContext.afterSignUpUrl} |
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.
❓ Not afterSignInUrl ?
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 within the isCombinedFlow routes, so those are the sign-up routes rendered within SignIn component therefore it uses signUpContext
izaaklauer
left a comment
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.
A relatively superficial review, but it looks correct and reasonable to me. Left a few comments. 👍
ad3a5d3 to
f8b7a53
Compare
Description
Resolves ORGS-581
Handles navigation to next tasks on an after-auth flow. Once all tasks get completed, it navigates to the
redirectUrlCompleteoption, which defaults to after sign-in/sign-up URL based on theClerk.clientstate.Developer flows
For AIO components, there's no signature changes and it'll work out of the box
For custom flows,
Clerk.nextTaskshould be called after performing a FAPI mutation that resolves a task. Here's an example for the upcoming force MFA task:Building your on custom React "onboarding" components:
CleanShot.2025-03-17.at.19.07.02-converted.mp4
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change