fix(clerk-js): Do not display organization list after creating organization throw tasks flow#6117
Conversation
🦋 Changeset detectedLatest commit: e01d4f5 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 Git ↗︎
|
📝 Walkthrough""" WalkthroughThis change refactors the organization selection and creation flow within the Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 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 (
|
LauraBeatris
changed the title
LauraBeatris
changed the title
LauraBeatris
force-pushed
the
laura/orgs-683-when-creating-a-first-organization-on-after-auth-clerkreact
branch
2 times, most recently
from
42c4bce to
7904177
Compare
LauraBeatris
force-pushed
the
laura/orgs-683-when-creating-a-first-organization-on-after-auth-clerkreact
branch
from
7904177 to
f1f5250
Compare
LauraBeatris
force-pushed
the
laura/orgs-683-when-creating-a-first-organization-on-after-auth-clerkreact
branch
from
f1f5250 to
c96f964
Compare
LauraBeatris
changed the title
LauraBeatris
force-pushed
the
laura/orgs-683-when-creating-a-first-organization-on-after-auth-clerkreact
branch
from
ff68c71 to
69324a6
Compare
LauraBeatris
changed the title
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/clerk-js/src/ui/components/OrganizationList/OrganizationListPage.tsx (1)
273-289: Add accessible status to the loading spinnerScreen-reader users receive no feedback during the blocking loading state.
Wrap the spinner in arole="status"container or passaria-label:- <Spinner - size={'lg'} - colorScheme={'primary'} - elementDescriptor={descriptors.spinner} - /> + <Spinner + size={'lg'} + colorScheme={'primary'} + elementDescriptor={descriptors.spinner} + role='status' + aria-label='Loading' + />This is a lightweight fix with zero visual impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/common-mice-ring.md(1 hunks)packages/clerk-js/src/ui/components/OrganizationList/OrganizationListPage.tsx(3 hunks)
🔇 Additional comments (1)
.changeset/common-mice-ring.md (1)
5-5: Changelog entry looks goodNo issues; entry is concise and clear.
packages/clerk-js/src/ui/components/OrganizationList/OrganizationListPage.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/OrganizationList/OrganizationListPage.tsx
Outdated
Show resolved
Hide resolved
LauraBeatris
force-pushed
the
laura/orgs-683-when-creating-a-first-organization-on-after-auth-clerkreact
branch
from
69324a6 to
d6269e6
Compare
LauraBeatris
force-pushed
the
laura/orgs-683-when-creating-a-first-organization-on-after-auth-clerkreact
branch
from
6a0b8fc to
75945a6
Compare
| const tracker = createBeforeUnloadTracker(this.#options.standardBrowser); | ||
| const defaultRedirectUrlComplete = this.client?.signUp ? this.buildAfterSignUpUrl() : this.buildAfterSignInUrl(); | ||
|
|
||
| this.#setTransitiveState(); |
There was a problem hiding this comment.
We do not need the transitive state here. It was causing the SignIn component to unmount due to withCoreSessionGuard that does not render its children when session === undefined
It makes sense when we're going from an empty session to an actual session, but since in this case we already have one, then I don't see the reason why to set a transitive state here.
There was a problem hiding this comment.
Here you can see SignIn getting unmounted due to this.#setTransitiveState();:
CleanShot.2025-06-18.at.11.44.35.mp4
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/clerk-js/src/ui/components/CreateOrganization/CreateOrganizationForm.tsx (1)
110-111: Drop the non-null assertion onlastCreatedOrganizationRef
lastCreatedOrganizationRef.current!is still asserted non-null even though the call is now optional.
Safer variant:-void props.navigateAfterCreateOrganization?.(lastCreatedOrganizationRef.current!); +const org = lastCreatedOrganizationRef.current; +if (org) { + void props.navigateAfterCreateOrganization?.(org); +}packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx (2)
1-3: Use the canonical import path for@clerk/shared/reactMixing
@clerk/shared/reactand@clerk/shared/react/indexcreates distinct module ids in bundlers and can bloat the bundle.-import { useOrganizationList } from '@clerk/shared/react/index'; +import { useOrganizationList } from '@clerk/shared/react';
124-125: EnsureisLoadingis always boolean
undefined || falseyieldsfalse, butundefined || undefinedyieldsundefined.
Cast explicitly to boolean to avoid tri-state surprises:-const isLoading = userMemberships?.isLoading || userInvitations?.isLoading || userSuggestions?.isLoading; +const isLoading = Boolean( + userMemberships?.isLoading || userInvitations?.isLoading || userSuggestions?.isLoading, +);packages/clerk-js/src/ui/components/SessionTasks/index.tsx (1)
16-18: Remove unusednavigatevariable
navigateis retrieved but never used insideSessionTasksStart, which will trigger lint warnings.- const { navigate } = useRouter(); + // no need to pull `navigate` from the router here
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/common-mice-ring.md(1 hunks)packages/clerk-js/src/core/clerk.ts(0 hunks)packages/clerk-js/src/ui/components/CreateOrganization/CreateOrganizationForm.tsx(5 hunks)packages/clerk-js/src/ui/components/OrganizationList/OrganizationListPage.tsx(2 hunks)packages/clerk-js/src/ui/components/SessionTasks/index.tsx(3 hunks)packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/clerk-js/src/core/clerk.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/common-mice-ring.md
- packages/clerk-js/src/ui/components/OrganizationList/OrganizationListPage.tsx
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: Integration Tests (nextjs, chrome, 13)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (tanstack-react-router, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (quickstart, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Static analysis
- GitHub Check: Unit Tests (18, --filter=@clerk/astro --filter=@clerk/backend --filter=@clerk/express --filter=@c...
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/clerk-js/src/ui/components/CreateOrganization/CreateOrganizationForm.tsx (1)
92-96: Early-return skips invitation wizard by design — double-check downstream expectationsCalling
sessionTasksContext.nextTask()and returning bypasses the invitation screen andcompleteFlow()logic.
Make sure every caller that providesSessionTasksContexttruly wants to skip invitations; otherwise the flow will silently change behaviour.
| sx={t => ({ | ||
| height: '100%', | ||
| minHeight: t.sizes.$60, | ||
| })} |
There was a problem hiding this comment.
We need to set a min-height on the loading state while the create organization form isn't rendered.
Ideally, the loading container should have a similar height as the create organization form / organization list, however it's not possible to know ahead of time since we need to wait for the useOrganizationList query to finish.
|
|
||
| return ( | ||
| <Card.Root> | ||
| <Card.Content sx={t => ({ padding: `${t.space.$8} ${t.space.$none} ${t.space.$none}` })}> |
There was a problem hiding this comment.
We're resetting the padding on the boundaries in order to have the divider:
Otherwise it'd stay like that:
| void userMemberships.revalidate?.(); | ||
|
|
||
| if (sessionTasksContext) { | ||
| await sessionTasksContext.nextTask(); |
There was a problem hiding this comment.
This is a pretty similar order as we'd have for custom flows:
- Create an organization
- Update it to the active session
- Verify if there are remaining tasks, and if so, navigate to the next one.
I've moved it inside of here now instead of passing it within onComplete prop so that we can await for it. onComplete is a sync property and I don't think we should turn into a promise in order to avoid side effects.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/clerk-js/src/ui/components/SessionTasks/index.tsx (1)
16-27: Remove the unusednavigatereference to avoid dead code & misleading dependencies
navigateis destructured fromuseRouter()but never used inside the component.
Keeping it solely to satisfy the dependency array is brittle and may hide real omissions later.- const { navigate } = useRouter(); + // No navigation needed here – we rely on Clerk’s internal helper. + const router = useRouter(); // keep a ref if you plan to log or debug, otherwise delete the line entirelyAlso trim the dependency list accordingly:
- }, [navigate, clerk, redirectUrlComplete]); + }, [clerk, redirectUrlComplete]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/clerk-js/bundlewatch.config.json(1 hunks)packages/clerk-js/src/ui/components/SessionTasks/index.tsx(3 hunks)packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/clerk-js/bundlewatch.config.json
- packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx
⏰ 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)
| { "path": "./dist/clerk.js", "maxSize": "610kB" }, | ||
| { "path": "./dist/clerk.browser.js", "maxSize": "70.16KB" }, | ||
| { "path": "./dist/clerk.legacy.browser.js", "maxSize": "113KB" }, | ||
| { "path": "./dist/clerk.js", "maxSize": "612kB" }, |
There was a problem hiding this comment.
The bundle increased around 4KB since I extracted the force-an-org flow outside of OrganizationList. It increased the general bundle, but we are lazy loading SessionTasks within SignIn.
I think the tradeoff is worth considering. I didn't want to introduce another if (sessionTasksContext) within OrganizationList to hijack the logic in there, but rather keep it separate.
Ideally, would be nice to use Suspense to only download the necessary after-auth steps according to Enviroment settings.
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx (1)
105-120: Loading state addresses previous feedback appropriately.The component includes the requested
minHeightstyling to prevent layout shifts during loading, addressing the concern raised in previous reviews.
🧹 Nitpick comments (1)
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx (1)
73-89: Component serves its purpose but consider consolidation.The
CreateOrganizationPagecomponent is simple and functional. However, given the complexity of conditional rendering across components, consider if this could be consolidated with similar logic inOrganizationSelectionPage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx (3)
1-12: Imports look appropriate for the component's functionality.All necessary dependencies are properly imported with clear separation between external libraries, internal contexts, and UI components.
17-35: Component logic is sound with proper loading and conditional rendering.The main task component correctly handles loading states and conditionally renders based on organization data availability. The use of
withCardStateProviderHOC ensures proper error handling context.
91-103: FlowCard component is well-structured with proper error handling.The component correctly uses card state context for error display and provides consistent styling across the flow.
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx (3)
37-67: LGTM - Clean conditional rendering implementation.The conditional rendering logic is well-structured and addresses the previous concerns about unreachable code. The state management for toggling between list and create form is handled correctly.
87-99: LGTM - Proper card wrapper with error handling.The FlowCard component correctly integrates with the card state provider and displays errors appropriately. The padding reset for the divider display aligns with the past review comments.
20-20: I’ll retry extracting the hook’s return shape with a properly labeled shell script:#!/bin/bash set -eux file=packages/shared/src/react/hooks/useOrganizationList.tsx echo "=== Hook signature and return block (around export function) ===" sed -n '245,350p' "$file" echo echo "=== All occurrences of “count” in this file (with context) ===" rg -n "count" -C3 "$file"
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/SessionTasks/tasks/ForceOrganizationSelection.tsx
Show resolved
Hide resolved
4 participants
Resolves ORGS-683, ORGS-705
Issue
Currently, when creating an organization during the force-an-org step, it re-renders
OrganizationListto display the list of memberships. This display is shown for a couple of seconds until the navigation toredirectUrlCompleteis complete.Solution
This PR introduces a separate chunk of UI for the
force-an-orgstep, in which gets rendered only if it's inside aSessionTasksflow. With that, it can run a different logic to not display the list of memberships after creation, but only if there are no organization memberships attached to the session.CleanShot.2025-06-18.at.11.36.50.mp4
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores